Date:   Fri, 12 Oct 2018 14:22:12 +0100
From:   Ben Dooks <ben.dooks@...ethink.co.uk>
To:     Bjørn Mork <bjorn@...k.no>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kernel@...ts.codethink.co.uk, gregkh@...uxfoundation.org,
        steve.glendinning@...well.net
Subject: Re: [PATCH 4/7] net: qmi_wwan: add usbnet -> priv function

On 12/10/18 11:44, Bjørn Mork wrote:
> Ben Dooks <ben.dooks@...ethink.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@...aculix:/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 ;-)

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Powered by blists - more mailing lists