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

From: "Bjørn Mork" <bjorn@mork.no>
To: Ben Dooks <ben.dooks@codethink.co.uk>
Cc: davem@davemloft.net, netdev@vger.kernel.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-kernel@lists.codethink.co.uk, gregkh@linuxfoundation.org,
	steve.glendinning@shawell.net
Subject: Re: [PATCH 4/7] net: qmi_wwan: add usbnet -> priv function
Date: Fri, 12 Oct 2018 12:44:57 +0200	[thread overview]
Message-ID: <87woqn30ly.fsf@miraculix.mork.no> (raw)
In-Reply-To: <20181012091642.21294-5-ben.dooks@codethink.co.uk> (Ben Dooks's message of "Fri, 12 Oct 2018 10:16:39 +0100")

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