Re: [PATCH 4/7] net: qmi_wwan: add usbnet -> priv function

[Ben Dooks]

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

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

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