| Message ID | 20181012091642.21294-5-ben.dooks@codethink.co.uk |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/7] net: smsc75xx: add usbnet -> priv function | expand |
On Fri, Oct 12, 2018 at 10:16:39AM +0100, Ben Dooks wrote: > There are a number of places in the qmi_wwan driver where it gets the > private-data from the usbnet passed in. It would be sensible to have > one inline function to convert it and change all points in the driver > to use that. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Ben Dooks <ben.dooks@codethink.co.uk> writes: > +static inline struct qmi_wwan_state *usbnet_to_qmi(struct usbnet *usb) > +{ > + return (void *) &usb->data; > +} checkpatch doesn't like this, and it is also inconsistent with the coding style elsewhere in the driver. CHECK: No space is necessary after a cast #30: FILE: drivers/net/usb/qmi_wwan.c:81: + return (void *) &usb->data; total: 0 errors, 0 warnings, 1 checks, 115 lines checked And as for consistency: I realize that "dev" is a very generic name, but it's used consistendly for all struct usbnet pointers in the driver: bjorn@miraculix:/usr/local/src/git/linux$ git grep 'struct usbnet' drivers/net/usb/qmi_wwan.c drivers/net/usb/qmi_wwan.c:static struct net_device *qmimux_find_dev(struct usbnet *dev, u8 mux_id) drivers/net/usb/qmi_wwan.c:static bool qmimux_has_slaves(struct usbnet *dev) drivers/net/usb/qmi_wwan.c:static int qmimux_rx_fixup(struct usbnet *dev, struct sk_buff *skb) drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(net); drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d)); drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d)); drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d)); drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d)); drivers/net/usb/qmi_wwan.c:static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb) drivers/net/usb/qmi_wwan.c:static int qmi_wwan_manage_power(struct usbnet *dev, int on) drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf); drivers/net/usb/qmi_wwan.c:static int qmi_wwan_register_subdriver(struct usbnet *dev) drivers/net/usb/qmi_wwan.c:static int qmi_wwan_change_dtr(struct usbnet *dev, bool on) drivers/net/usb/qmi_wwan.c:static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf) drivers/net/usb/qmi_wwan.c: BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) < drivers/net/usb/qmi_wwan.c:static void qmi_wwan_unbind(struct usbnet *dev, struct usb_interface *intf) drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf); drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf); drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf); The name was chosen to be consistent with the cdc_ether usage. I'd prefer it if this function could use the "dev" name, to avoid confusing things unnecessarly with yet another generic name like "usb". Which isn't used anywhere else in the driver, and could just as easily refer to a struct usb_device or struct usb_interface. (yes, I know there are a couple of "struct usb_device *dev" cases. But I'd rather see those renamed TBH) I also don't see why this function should be qmi_wwan specific. No need to duplicate the same function in every minidriver, when you can do with two generic helpers (maybe with more meaningful names): static inline void *usbnet_priv(const struct usbnet *dev) { return (void *)&dev->data; } static inline void *usbnet_priv0(const struct usbnet * *dev) { return (void *)dev->data[0]; } Please fix the checkpatch warning and consider the other comments. Personally I don't really think it makes the code any easier to read. But if you want it, then I'm fine this. I guess I've grown too used to seeing those casts ;-) Bjørn
On 12/10/18 11:44, Bjørn Mork wrote: > Ben Dooks <ben.dooks@codethink.co.uk> writes: > >> +static inline struct qmi_wwan_state *usbnet_to_qmi(struct usbnet *usb) >> +{ >> + return (void *) &usb->data; >> +} > > > checkpatch doesn't like this, and it is also inconsistent with the > coding style elsewhere in the driver. Thank you for pointing this out, will fix in v2. > CHECK: No space is necessary after a cast > #30: FILE: drivers/net/usb/qmi_wwan.c:81: > + return (void *) &usb->data; > > total: 0 errors, 0 warnings, 1 checks, 115 lines checked > > > And as for consistency: I realize that "dev" is a very generic name, > but it's used consistendly for all struct usbnet pointers in the driver: Ok, I'll change it. > > bjorn@miraculix:/usr/local/src/git/linux$ git grep 'struct usbnet' drivers/net/usb/qmi_wwan.c > drivers/net/usb/qmi_wwan.c:static struct net_device *qmimux_find_dev(struct usbnet *dev, u8 mux_id) > drivers/net/usb/qmi_wwan.c:static bool qmimux_has_slaves(struct usbnet *dev) > drivers/net/usb/qmi_wwan.c:static int qmimux_rx_fixup(struct usbnet *dev, struct sk_buff *skb) > drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(net); > drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d)); > drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d)); > drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d)); > drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d)); > drivers/net/usb/qmi_wwan.c:static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb) > drivers/net/usb/qmi_wwan.c:static int qmi_wwan_manage_power(struct usbnet *dev, int on) > drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf); > drivers/net/usb/qmi_wwan.c:static int qmi_wwan_register_subdriver(struct usbnet *dev) > drivers/net/usb/qmi_wwan.c:static int qmi_wwan_change_dtr(struct usbnet *dev, bool on) > drivers/net/usb/qmi_wwan.c:static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf) > drivers/net/usb/qmi_wwan.c: BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) < > drivers/net/usb/qmi_wwan.c:static void qmi_wwan_unbind(struct usbnet *dev, struct usb_interface *intf) > drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf); > drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf); > drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf); > > The name was chosen to be consistent with the cdc_ether usage. I'd > prefer it if this function could use the "dev" name, to avoid confusing > things unnecessarly with yet another generic name like "usb". Which > isn't used anywhere else in the driver, and could just as easily refer > to a struct usb_device or struct usb_interface. Ok, should be fairly easy to change this. > (yes, I know there are a couple of "struct usb_device *dev" cases. But > I'd rather see those renamed TBH) I'd rather not touch that at the moment, this is already gaining complexity. > I also don't see why this function should be qmi_wwan specific. No need > to duplicate the same function in every minidriver, when you can do with > two generic helpers (maybe with more meaningful names): I personally prefer the return type to be specifically the private data type for the driver. It makes it a bit easier to spot when you don't get the cast right. The functions below are the idea I am working towards for the usbnet driver, I was trying to avoid doing everything in one go. Making the accessor functions means the next round of changes can be much smaller, touching 1-2 areas per driver. > static inline void *usbnet_priv(const struct usbnet *dev) > { > return (void *)&dev->data; > } > > static inline void *usbnet_priv0(const struct usbnet * *dev) > { > return (void *)dev->data[0]; > } This is probably the end-game of the patch series, just need to look at a couple of issues and see if there's anything better than can be done. I might also add a usbnet_set_privdata(dev, data) or something like usbnet_alloc_privdata(dev, sizeof(data). Note, there's also the fun here of the usbnet having private data for itself, and these sub-drivers also having their own private data... this probably needs to be named carefully. > Please fix the checkpatch warning and consider the other comments. > > Personally I don't really think it makes the code any easier to read. > But if you want it, then I'm fine this. I guess I've grown too used to > seeing those casts ;-)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c index 533b6fb8d923..45930758a945 100644 --- a/drivers/net/usb/qmi_wwan.c +++ b/drivers/net/usb/qmi_wwan.c @@ -76,6 +76,11 @@ struct qmimux_priv { u8 mux_id; }; +static inline struct qmi_wwan_state *usbnet_to_qmi(struct usbnet *usb) +{ + return (void *) &usb->data; +} + static int qmimux_open(struct net_device *dev) { struct qmimux_priv *priv = netdev_priv(dev); @@ -253,7 +258,7 @@ static void qmimux_unregister_device(struct net_device *dev) static void qmi_wwan_netdev_setup(struct net_device *net) { struct usbnet *dev = netdev_priv(net); - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); if (info->flags & QMI_WWAN_FLAG_RAWIP) { net->header_ops = NULL; /* No header */ @@ -276,7 +281,7 @@ static void qmi_wwan_netdev_setup(struct net_device *net) static ssize_t raw_ip_show(struct device *d, struct device_attribute *attr, char *buf) { struct usbnet *dev = netdev_priv(to_net_dev(d)); - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); return sprintf(buf, "%c\n", info->flags & QMI_WWAN_FLAG_RAWIP ? 'Y' : 'N'); } @@ -284,7 +289,7 @@ static ssize_t raw_ip_show(struct device *d, struct device_attribute *attr, char static ssize_t raw_ip_store(struct device *d, struct device_attribute *attr, const char *buf, size_t len) { struct usbnet *dev = netdev_priv(to_net_dev(d)); - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); bool enable; int ret; @@ -346,7 +351,7 @@ static ssize_t add_mux_show(struct device *d, struct device_attribute *attr, cha static ssize_t add_mux_store(struct device *d, struct device_attribute *attr, const char *buf, size_t len) { struct usbnet *dev = netdev_priv(to_net_dev(d)); - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); u8 mux_id; int ret; @@ -391,7 +396,7 @@ static ssize_t del_mux_show(struct device *d, struct device_attribute *attr, cha static ssize_t del_mux_store(struct device *d, struct device_attribute *attr, const char *buf, size_t len) { struct usbnet *dev = netdev_priv(to_net_dev(d)); - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); struct net_device *del_dev; u8 mux_id; int ret = 0; @@ -468,7 +473,7 @@ static const u8 buggy_fw_addr[ETH_ALEN] = {0x00, 0xa0, 0xc6, 0x00, 0x00, 0x00}; */ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb) { - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); bool rawip = info->flags & QMI_WWAN_FLAG_RAWIP; __be16 proto; @@ -555,7 +560,7 @@ static const struct net_device_ops qmi_wwan_netdev_ops = { */ static int qmi_wwan_manage_power(struct usbnet *dev, int on) { - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); int rv; dev_dbg(&dev->intf->dev, "%s() pmcount=%d, on=%d\n", __func__, @@ -589,7 +594,7 @@ static int qmi_wwan_register_subdriver(struct usbnet *dev) { int rv; struct usb_driver *subdriver = NULL; - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); /* collect bulk endpoints */ rv = usbnet_get_endpoints(dev, info->data); @@ -651,7 +656,7 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf) struct usb_cdc_union_desc *cdc_union; struct usb_cdc_ether_desc *cdc_ether; struct usb_driver *driver = driver_of(intf); - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); struct usb_cdc_parsed_header hdr; BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) < @@ -746,7 +751,7 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf) static void qmi_wwan_unbind(struct usbnet *dev, struct usb_interface *intf) { - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); struct usb_driver *driver = driver_of(intf); struct usb_interface *other; @@ -785,7 +790,7 @@ static void qmi_wwan_unbind(struct usbnet *dev, struct usb_interface *intf) static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message) { struct usbnet *dev = usb_get_intfdata(intf); - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); int ret; /* Both usbnet_suspend() and subdriver->suspend() MUST return 0 @@ -808,7 +813,7 @@ static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message) static int qmi_wwan_resume(struct usb_interface *intf) { struct usbnet *dev = usb_get_intfdata(intf); - struct qmi_wwan_state *info = (void *)&dev->data; + struct qmi_wwan_state *info = usbnet_to_qmi(dev); int ret = 0; bool callsub = (intf == info->control && info->subdriver && info->subdriver->resume); @@ -1406,7 +1411,7 @@ static void qmi_wwan_disconnect(struct usb_interface *intf) /* called twice if separate control and data intf */ if (!dev) return; - info = (void *)&dev->data; + info = usbnet_to_qmi(dev); if (info->flags & QMI_WWAN_FLAG_MUX) { if (!rtnl_trylock()) { restart_syscall();
There are a number of places in the qmi_wwan driver where it gets the private-data from the usbnet passed in. It would be sensible to have one inline function to convert it and change all points in the driver to use that. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- drivers/net/usb/qmi_wwan.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)