Re: [RFC net-next 1/3] net: usb: divorce private data and cdc state in usbnet

["Bjørn Mork" <bjorn@mork.no>]

Oliver Neukum <oneukum@suse.com> writes:

> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -16,6 +16,18 @@
>  #include <linux/usb.h>
>  #include <linux/spinlock.h>
>  
> +/* Drivers that reuse some of the standard USB CDC infrastructure
> + * (notably, using multiple interfaces according to the CDC
> + * union descriptor) get some helper code.
> + */
> +struct cdc_state {
> +        struct usb_cdc_header_desc      *header;
> +        struct usb_cdc_union_desc       *u;
> +        struct usb_cdc_ether_desc       *ether;
> +        struct usb_interface            *control;
> +        struct usb_interface            *data;
> +};
> +
>  /* interface from usbnet core to each USB networking link we handle */
>  struct usbnet {
>  	/* housekeeping */
> @@ -41,7 +53,7 @@ struct usbnet {
>  	/* protocol/interface state */
>  	struct net_device	*net;
>  	int			msg_enable;
> -	unsigned long		data[5];
> +	struct cdc_state	cdc;		/* too common to leave out*/
>  	u32			xid;
>  	u32			hard_mtu;	/* count any extra framing */
>  	size_t			rx_urb_size;	/* size for rx urbs */
> @@ -84,6 +96,7 @@ struct usbnet {
>   * that must be broken
>   */
>  #		define EVENT_UNPLUG		31
> +	unsigned long		private[5];
>  };
>  
>  static inline bool usbnet_going_away(struct usbnet *ubn)

I like the idea.  Nice to get rid of those extra allocations for drivers
needing more than 5 pointers.

But I don't understand why you make struct cdc_state special.  That's
just confusing.

I know I intentionally made the "control" and "data" fields of
"struct qmi_wwan_state" position and name compatible with
"struct cdc_state". Maybe not very important.  But adds to the
confusion if we end up with a double set of those fields.

Or should the qmi_wwan driver split out the 
        struct usb_interface            *control;
        struct usb_interface            *data;

part of its private data and put those into "cdc", allowing us to reduce
"struct qmi_wwan_state" from 5 to 3 longs?  I don't think that's a very
good idea...

Let's keep this a single private minidriver allocation.  Making it
flexible is nice.


Bjørn