| Message ID | 20220504142006.3804-1-m.chetan.kumar@linux.intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: wwan: fix port open | expand |
Hi Chetan, On Wed, 4 May 2022 at 16:09, M Chetan Kumar <m.chetan.kumar@linux.intel.com> wrote: > > Wwan device registered port can be opened as many number of times. > The first port open() call binds dev file to driver wwan port device > and subsequent open() call references to same wwan port instance. > > When dev file is opened multiple times, all contexts still refers to > same instance of wwan port. So in tx path, the received data will be > fwd to wwan device but in rx path the wwan port has a single rx queue. > Depending on which context goes for early read() the rx data gets > dispatched to it. > > Since the wwan port is not handling dispatching of rx data to right > context restrict wwan port open to single context. The reason for this behavior comes from: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2313348.html Especially: --- "I told you before, do not try to keep a device node from being opened multiple times, as it will always fail (think about passing file handles around between programs...) If userspace wants to do this, it will do it. If your driver can't handle that, that's fine, userspace will learn not to do that. But the kernel can not prevent this from happening." --- So maybe a user-side solution would be more appropriate, /var/lock/ ? Regards, Loic > > Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com> > --- > drivers/net/wwan/wwan_core.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c > index b8c7843730ed..9ca2d8d76587 100644 > --- a/drivers/net/wwan/wwan_core.c > +++ b/drivers/net/wwan/wwan_core.c > @@ -33,6 +33,7 @@ static struct dentry *wwan_debugfs_dir; > > /* WWAN port flags */ > #define WWAN_PORT_TX_OFF 0 > +#define WWAN_PORT_OPEN 1 > > /** > * struct wwan_device - The structure that defines a WWAN device > @@ -58,7 +59,6 @@ struct wwan_device { > /** > * struct wwan_port - The structure that defines a WWAN port > * @type: Port type > - * @start_count: Port start counter > * @flags: Store port state and capabilities > * @ops: Pointer to WWAN port operations > * @ops_lock: Protect port ops > @@ -70,7 +70,6 @@ struct wwan_device { > */ > struct wwan_port { > enum wwan_port_type type; > - unsigned int start_count; > unsigned long flags; > const struct wwan_port_ops *ops; > struct mutex ops_lock; /* Serialize ops + protect against removal */ > @@ -496,7 +495,7 @@ void wwan_remove_port(struct wwan_port *port) > struct wwan_device *wwandev = to_wwan_dev(port->dev.parent); > > mutex_lock(&port->ops_lock); > - if (port->start_count) > + if (test_and_clear_bit(WWAN_PORT_OPEN, &port->flags)) > port->ops->stop(port); > port->ops = NULL; /* Prevent any new port operations (e.g. from fops) */ > mutex_unlock(&port->ops_lock); > @@ -549,11 +548,14 @@ static int wwan_port_op_start(struct wwan_port *port) > } > > /* If port is already started, don't start again */ > - if (!port->start_count) > - ret = port->ops->start(port); > + if (test_bit(WWAN_PORT_OPEN, &port->flags)) { > + ret = -EBUSY; > + goto out_unlock; > + } > + ret = port->ops->start(port); > > if (!ret) > - port->start_count++; > + set_bit(WWAN_PORT_OPEN, &port->flags); > > out_unlock: > mutex_unlock(&port->ops_lock); > @@ -564,8 +566,7 @@ static int wwan_port_op_start(struct wwan_port *port) > static void wwan_port_op_stop(struct wwan_port *port) > { > mutex_lock(&port->ops_lock); > - port->start_count--; > - if (!port->start_count) { > + if (test_and_clear_bit(WWAN_PORT_OPEN, &port->flags)) { > if (port->ops) > port->ops->stop(port); > skb_queue_purge(&port->rxq); > -- > 2.25.1 >
Hi Loic, On 5/4/2022 8:14 PM, Loic Poulain wrote: > Hi Chetan, > > On Wed, 4 May 2022 at 16:09, M Chetan Kumar > <m.chetan.kumar@linux.intel.com> wrote: >> >> Wwan device registered port can be opened as many number of times. >> The first port open() call binds dev file to driver wwan port device >> and subsequent open() call references to same wwan port instance. >> >> When dev file is opened multiple times, all contexts still refers to >> same instance of wwan port. So in tx path, the received data will be >> fwd to wwan device but in rx path the wwan port has a single rx queue. >> Depending on which context goes for early read() the rx data gets >> dispatched to it. >> >> Since the wwan port is not handling dispatching of rx data to right >> context restrict wwan port open to single context. > > The reason for this behavior comes from: > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2313348.html > > Especially: > --- > "I told you before, do not try to keep a device node from being opened > multiple times, as it will always fail (think about passing file > handles around between programs...) If userspace wants to do this, it > will do it. If your driver can't handle that, that's fine, userspace > will learn not to do that. But the kernel can not prevent this from > happening." > --- > > So maybe a user-side solution would be more appropriate, /var/lock/ ? OK. So this means user space application like modem manager should lock the wwan port so that wwan port will not be available for other programs ? Regards, Chetan
diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c index b8c7843730ed..9ca2d8d76587 100644 --- a/drivers/net/wwan/wwan_core.c +++ b/drivers/net/wwan/wwan_core.c @@ -33,6 +33,7 @@ static struct dentry *wwan_debugfs_dir; /* WWAN port flags */ #define WWAN_PORT_TX_OFF 0 +#define WWAN_PORT_OPEN 1 /** * struct wwan_device - The structure that defines a WWAN device @@ -58,7 +59,6 @@ struct wwan_device { /** * struct wwan_port - The structure that defines a WWAN port * @type: Port type - * @start_count: Port start counter * @flags: Store port state and capabilities * @ops: Pointer to WWAN port operations * @ops_lock: Protect port ops @@ -70,7 +70,6 @@ struct wwan_device { */ struct wwan_port { enum wwan_port_type type; - unsigned int start_count; unsigned long flags; const struct wwan_port_ops *ops; struct mutex ops_lock; /* Serialize ops + protect against removal */ @@ -496,7 +495,7 @@ void wwan_remove_port(struct wwan_port *port) struct wwan_device *wwandev = to_wwan_dev(port->dev.parent); mutex_lock(&port->ops_lock); - if (port->start_count) + if (test_and_clear_bit(WWAN_PORT_OPEN, &port->flags)) port->ops->stop(port); port->ops = NULL; /* Prevent any new port operations (e.g. from fops) */ mutex_unlock(&port->ops_lock); @@ -549,11 +548,14 @@ static int wwan_port_op_start(struct wwan_port *port) } /* If port is already started, don't start again */ - if (!port->start_count) - ret = port->ops->start(port); + if (test_bit(WWAN_PORT_OPEN, &port->flags)) { + ret = -EBUSY; + goto out_unlock; + } + ret = port->ops->start(port); if (!ret) - port->start_count++; + set_bit(WWAN_PORT_OPEN, &port->flags); out_unlock: mutex_unlock(&port->ops_lock); @@ -564,8 +566,7 @@ static int wwan_port_op_start(struct wwan_port *port) static void wwan_port_op_stop(struct wwan_port *port) { mutex_lock(&port->ops_lock); - port->start_count--; - if (!port->start_count) { + if (test_and_clear_bit(WWAN_PORT_OPEN, &port->flags)) { if (port->ops) port->ops->stop(port); skb_queue_purge(&port->rxq);
Wwan device registered port can be opened as many number of times. The first port open() call binds dev file to driver wwan port device and subsequent open() call references to same wwan port instance. When dev file is opened multiple times, all contexts still refers to same instance of wwan port. So in tx path, the received data will be fwd to wwan device but in rx path the wwan port has a single rx queue. Depending on which context goes for early read() the rx data gets dispatched to it. Since the wwan port is not handling dispatching of rx data to right context restrict wwan port open to single context. Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com> --- drivers/net/wwan/wwan_core.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)