RE: [RFC 1/4] Add usb_get_address and usb_set_address support

From: <Charles.Hyde@dellteam.com>
To: <oneukum@suse.com>, <gregkh@linuxfoundation.org>
Cc: <Mario.Limonciello@dell.com>, <nic_swsd@realtek.com>,
	<linux-acpi@vger.kernel.org>, <linux-usb@vger.kernel.org>,
	<netdev@vger.kernel.org>
Subject: RE: [RFC 1/4] Add usb_get_address and usb_set_address support
Date: Thu, 22 Aug 2019 17:14:30 +0000	[thread overview]
Message-ID: <8014f932039c4e01bd513148f20ca0e4@AUSX13MPS303.AMER.DELL.COM> (raw)
In-Reply-To: <1566461295.8347.19.camel@suse.com>

> > <snipped>
> > >
> > > This is a VERY cdc-net-specific function.  It is not a "generic" USB
> > > function at all.  Why does it belong in the USB core?  Shouldn't it
> > > live in the code that handles the other cdc-net-specific logic?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> >
> > Thank you for this feedback, Greg.  I was not sure about adding this to
> message.c, because of the USB_CDC_GET_NET_ADDRESS.  I had found
> references to SET_ADDRESS in the USB protocol at
> https://wiki.osdev.org/Universal_Serial_Bus#USB_Protocol.  If one wanted a
> generic USB function for SET_ADDRESS, to be used for both sending a MAC
> address and receiving one, how would you suggest this be implemented?  This
> is a legit question because I am curious.
> 
> Your implementation was, except for missing error handling, usable.
> The problem is where you put it. CDC messages exist only for CDC devices. Now
> it is true that there is no generic CDC driver.
> Creating a module just for that would cost more memory than it saves in most
> cases.
> But MACs are confined to network devices. Hence the functionality can be put
> into usbnet. It should not be put into any individual driver, so that every
> network driver can use it without duplication.
> 
> > Your feedback led to moving the functionality into cdc_ncm.c for today's
> testing, and removing all changes from messages.c, usb.h, usbnet.c, and
> usbnet.h.  This may be where I end up long term, but I would like to learn if
> there is a possible solution that could live in message.c and be callable from
> other USB-to-Ethernet aware drivers.
> 
> All those drivers use usbnet. Hence there it should be.
> 
> 	Regards
> 		Oliver

Some of the drivers in drivers/net/usb/ do call functions in drivers/net/usb/usbnet, but not all.  As Greg pointed out, the USB change I developed is cdc specific, so putting it into usbnet would raise the same concerns Greg mentioned.  Leaving my newest implementation in cdc_ncm.c will be most appropriate, as it also fits with what other drivers in this folder have done.  My original code was rather short sighted, at best.

Charles