Re: [PATCH v2 15/17] usb: gadget: ether: Use plain udevice for UDC controller interaction

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

On lun., sept. 04, 2023 at 11:10, Marek Vasut <marex@denx.de> wrote:

> On 9/4/23 09:56, Mattijs Korpershoek wrote:
>> On ven., sept. 01, 2023 at 11:50, Marek Vasut <marex@denx.de> wrote:
>> 
>>> Convert to plain udevice interaction with UDC controller
>>> device, avoid the use of UDC uclass dev_array .
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> ---
>>> Cc: Angus Ainslie <angus@akkea.ca>
>>> Cc: Dmitrii Merkurev <dimorinny@google.com>
>>> Cc: Eddie Cai <eddie.cai.linux@gmail.com>
>>> Cc: Kever Yang <kever.yang@rock-chips.com>
>>> Cc: Lukasz Majewski <lukma@denx.de>
>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>>> Cc: Nishanth Menon <nm@ti.com>
>>> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Stefan Roese <sr@denx.de>
>>> Cc: kernel@puri.sm
>>> ---
>>> V2: Use udev->parent for UDC access
>>> ---
>>>   drivers/usb/gadget/ether.c | 20 +++++++++++---------
>>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
>>> index 5ff06d3814b..11b1a6221b3 100644
>>> --- a/drivers/usb/gadget/ether.c
>>> +++ b/drivers/usb/gadget/ether.c
>>> @@ -1880,8 +1880,10 @@ static void eth_start(struct eth_dev *dev, gfp_t gfp_flags)
>>>   	}
>>>   }
>>>   
>>> -static int eth_stop(struct eth_dev *dev)
>>> +static int eth_stop(struct udevice *udev)
>>>   {
>>> +	struct ether_priv *priv = dev_get_priv(udev);
>>> +	struct eth_dev *dev = &priv->ethdev;
>>>   #ifdef RNDIS_COMPLETE_SIGNAL_DISCONNECT
>>>   	unsigned long ts;
>>>   	unsigned long timeout = CONFIG_SYS_HZ; /* 1 sec to stop RNDIS */
>>> @@ -1895,7 +1897,7 @@ static int eth_stop(struct eth_dev *dev)
>>>   		/* Wait until host receives OID_GEN_MEDIA_CONNECT_STATUS */
>>>   		ts = get_timer(0);
>>>   		while (get_timer(ts) < timeout)
>>> -			usb_gadget_handle_interrupts(0);
>>> +			dm_usb_gadget_handle_interrupts(udev->parent);
>>>   #endif
>>>   
>>>   		rndis_uninit(dev->rndis_config);
>>> @@ -2300,7 +2302,7 @@ static int usb_eth_start(struct udevice *udev)
>>>   			pr_err("The remote end did not respond in time.");
>>>   			goto fail;
>>>   		}
>>> -		usb_gadget_handle_interrupts(0);
>>> +		dm_usb_gadget_handle_interrupts(udev->parent);
>>>   	}
>>>   
>>>   	packet_received = 0;
>>> @@ -2370,7 +2372,7 @@ static int usb_eth_send(struct udevice *udev, void *packet, int length)
>>>   			printf("timeout sending packets to usb ethernet\n");
>>>   			return -1;
>>>   		}
>>> -		usb_gadget_handle_interrupts(0);
>>> +		dm_usb_gadget_handle_interrupts(udev->parent);
>>>   	}
>>>   	free(rndis_pkt);
>>>   
>>> @@ -2400,13 +2402,13 @@ static void usb_eth_stop(struct udevice *udev)
>>>   	 * 2) 'pullup' callback in your UDC driver can be improved to perform
>>>   	 * this deinitialization.
>>>   	 */
>>> -	eth_stop(dev);
>>> +	eth_stop(udev);
>>>   
>>>   	usb_gadget_disconnect(dev->gadget);
>>>   
>>>   	/* Clear pending interrupt */
>>>   	if (dev->network_started) {
>>> -		usb_gadget_handle_interrupts(0);
>>> +		dm_usb_gadget_handle_interrupts(udev->parent);
>>>   		dev->network_started = 0;
>>>   	}
>>>   }
>>> @@ -2416,7 +2418,7 @@ static int usb_eth_recv(struct udevice *dev, int flags, uchar **packetp)
>>>   	struct ether_priv *priv = dev_get_priv(dev);
>>>   	struct eth_dev *ethdev = &priv->ethdev;
>>>   
>>> -	usb_gadget_handle_interrupts(0);
>>> +	dm_usb_gadget_handle_interrupts(dev->parent);
>>>   
>>>   	if (packet_received) {
>>>   		if (ethdev->rx_req) {
>>> @@ -2467,7 +2469,7 @@ int usb_ether_init(void)
>>>   		return ret;
>>>   	}
>>>   
>>> -	return usb_gadget_initialize(0);
>>> +	return 0;
>> 
>> No udc_device_get() here?
>> 
>> This looks a little suspicious, because in the unbind() we call
>> udc_device_put().
>
> Look at how udc_device_get_by_index() is implemented and what it does, 
> compared to what usb_ether_init() does (hint: they are basically doing 
> the same thing). This udc_device_put() here is only a workaround for 
> platforms which are not using the bind command to bind USB ethernet yet.

Oh right. Thank you for explaining.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>