| Message ID | 20210125152235.2942-2-dnlplm@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,1/2] net: usb: qmi_wwan: add qmap id sysfs file for qmimux interfaces | expand |
On Mon, Jan 25, 2021 at 4:23 PM Daniele Palmas <dnlplm@gmail.com> wrote: > > Add qmimux interface sysfs file qmap/mux_id to show qmap id set > during the interface creation, in order to provide a method for > userspace to associate QMI control channels to network interfaces. > > Signed-off-by: Daniele Palmas <dnlplm@gmail.com> Thanks for doing this! Acked-by: Aleksander Morgado <aleksander@aleksander.es>
On Mon, 25 Jan 2021 17:14:28 +0100 Bjørn Mork wrote: > Daniele Palmas <dnlplm@gmail.com> writes: > > > Add qmimux interface sysfs file qmap/mux_id to show qmap id set > > during the interface creation, in order to provide a method for > > userspace to associate QMI control channels to network interfaces. > > > > Signed-off-by: Daniele Palmas <dnlplm@gmail.com> > > Acked-by: Bjørn Mork <bjorn@mork.no> We got two patches adding new sysfs files for QMI in close succession - is there a sense of how much this interface will grow over time? It's no secret that we prefer netlink in networking land.
On Mon, Jan 25, 2021 at 04:22:34PM +0100, Daniele Palmas wrote: > Add qmimux interface sysfs file qmap/mux_id to show qmap id set > during the interface creation, in order to provide a method for > userspace to associate QMI control channels to network interfaces. > > Signed-off-by: Daniele Palmas <dnlplm@gmail.com> > --- > drivers/net/usb/qmi_wwan.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c > index 7ea113f51074..9b85e2ed4760 100644 > --- a/drivers/net/usb/qmi_wwan.c > +++ b/drivers/net/usb/qmi_wwan.c > @@ -218,6 +218,31 @@ static int qmimux_rx_fixup(struct usbnet *dev, struct sk_buff *skb) > return 1; > } > > +static ssize_t mux_id_show(struct device *d, struct device_attribute *attr, char *buf) > +{ > + struct net_device *dev = to_net_dev(d); > + struct qmimux_priv *priv; > + ssize_t count = 0; > + > + priv = netdev_priv(dev); > + count += scnprintf(&buf[count], PAGE_SIZE - count, > + "0x%02x\n", priv->mux_id); Odd way to do this, please just use sysfs_emit(). It looks like you cut/pasted this from some other more complex logic. thanks, greg k-h
On Wed, 27 Jan 2021 08:26:13 +0100 Bjørn Mork wrote: > Jakub Kicinski <kuba@kernel.org> writes: > > We got two patches adding new sysfs files for QMI in close succession - > > is there a sense of how much this interface will grow over time? > > The honest answer is no. > > I do not expect this interface to grow at all. But then I didn't expect > it to grow before the two recent additions either... Both are results > of feedback from the userspace developers actually using this interface. > > If I try to look into the future, then I do believe the first addition, > the "pass_through" flag, makes further changes unnecessary. It allows > the "rmnet" driver to take over all the functionality related to > qmap/qmimux. The rmnet driver has a proper netlink interface for > management. This is how the design should have been from the start, and > would have been if the "rmnet" driver had existed when we added qmap > support to qmi_wwan. Or if I had been aware that someone was working on > such a driver. > > So why do we still need this last addition discussed here? Well, there > are users of the qmi_wwan internal qmimux interface. They should move > to "rmnet", but this might take some time and we obviously can't remove > the old interface in any case. But there is a design flaw in that > interface, which makes it rather difficult to use. This last addition > fixes that flaw. > > I'll definitely accept the judgement if you want to put your foot down > and say that this has to stop here, and that we are better served > without this last fix. > > > It's no secret that we prefer netlink in networking land. > > Yes. But given that we have the sysfs interface for managing this > qmimux feature, I don't see netlink as an alternative to this patch. > > The same really applies to the previous sysfs attribute, adding another > flag to a set which is already exposed as sysfs attributes. > > The good news is that it allowed further qmimux handling to be offloaded > to "rmnet", which does have a netlink interface. Thanks for the explanation. I'll trust you on this one :) I applied v2 and added the acks from v1.
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c index 7ea113f51074..9b85e2ed4760 100644 --- a/drivers/net/usb/qmi_wwan.c +++ b/drivers/net/usb/qmi_wwan.c @@ -218,6 +218,31 @@ static int qmimux_rx_fixup(struct usbnet *dev, struct sk_buff *skb) return 1; } +static ssize_t mux_id_show(struct device *d, struct device_attribute *attr, char *buf) +{ + struct net_device *dev = to_net_dev(d); + struct qmimux_priv *priv; + ssize_t count = 0; + + priv = netdev_priv(dev); + count += scnprintf(&buf[count], PAGE_SIZE - count, + "0x%02x\n", priv->mux_id); + + return count; +} + +static DEVICE_ATTR_RO(mux_id); + +static struct attribute *qmi_wwan_sysfs_qmimux_attrs[] = { + &dev_attr_mux_id.attr, + NULL, +}; + +static struct attribute_group qmi_wwan_sysfs_qmimux_attr_group = { + .name = "qmap", + .attrs = qmi_wwan_sysfs_qmimux_attrs, +}; + static int qmimux_register_device(struct net_device *real_dev, u8 mux_id) { struct net_device *new_dev; @@ -240,6 +265,8 @@ static int qmimux_register_device(struct net_device *real_dev, u8 mux_id) goto out_free_newdev; } + new_dev->sysfs_groups[0] = &qmi_wwan_sysfs_qmimux_attr_group; + err = register_netdevice(new_dev); if (err < 0) goto out_free_newdev;
Add qmimux interface sysfs file qmap/mux_id to show qmap id set during the interface creation, in order to provide a method for userspace to associate QMI control channels to network interfaces. Signed-off-by: Daniele Palmas <dnlplm@gmail.com> --- drivers/net/usb/qmi_wwan.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)