[PATCH v2 4/6] usb: Add environment based device blocklist

[Marek Vasut]

On 3/17/24 7:15 PM, Janne Grunau wrote:
> Hej,

Hi,

> On Sun, Mar 17, 2024, at 17:18, Marek Vasut wrote:
>> On 3/17/24 12:07 PM, Janne Grunau via B4 Relay wrote:
>>> From: Janne Grunau <j at jannau.net>
>>>
>>> Add the environment variable "usb_blocklist" to prevent USB devices
>>> listed in it from being used. This allows to ignore devices which
>>> trigger bugs in u-boot's USB stack or are undesirable for other reasons.
>>> Devices emulating keyboards are one example. U-boot currently supports
>>> only one USB keyboard device. Most commonly, people run into this with
>>> Yubikeys, so let's ignore those in the default environment.
>>>
>>> Based on previous USB keyboard specific patches for the same purpose.
>>>
>>> Link: https://lore.kernel.org/u-boot/7ab604fb-0fec-4f5e-8708-7a3a7e2cb568@denx.de/
>>> Signed-off-by: Janne Grunau <j at jannau.net>
>>> ---
>>>    common/usb.c              | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>>>    doc/usage/environment.rst | 12 ++++++++++
>>>    include/env_default.h     | 11 ++++++++++
>>>    3 files changed, 79 insertions(+)
>>>
>>> diff --git a/common/usb.c b/common/usb.c
>>> index 836506dcd9..73af5be066 100644
>>> --- a/common/usb.c
>>> +++ b/common/usb.c
>>> @@ -1084,6 +1084,57 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
>>>    	return 0;
>>>    }
>>>    
>>> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
>>> +{
>>> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
>>> +	       blocklist);
>>> +	return 0;
>>
>> This could be static void without return 0 at the end.
> 
> the return is there to break out of the while loop on parsing errors in a single statement. This probably won't be necessary after using strsep and sscanf in the parsing function but see below.

Ahh, now I see it. But then, shouldn't this return -ENODEV here already ?

>>> +}
>>> +
>>> +static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
>>> +{
>>> +	ulong vid, pid;
>>> +	char *end;
>>> +	const char *block_str = env_get("usb_blocklist");
>>> +	const char *cur = block_str;
>>> +
>>> +	/* parse "usb_blocklist" strictly */
>>> +	while (cur && cur[0] != '\0') {
>>
>> Have a look at strsep() , namely strsep(block_str, ","); This will split
>> the string up for you at "," delimiters.
>>
>> Example is in drivers/dfu/dfu.c dfu_config_interfaces() .
> 
> strsep() is probably a good idea even if it alone won't make the code that much simpler for strict parsing.
> 
>> And then, on each token, you can try and run sscanf(token, "%04x:%04x",
>> vid, pid);, that will parse the token format for you. See e.g.
>> test/lib/sscanf.c for further examples.
>>
>> That should simplify the parsing a lot.
> 
> It would but sscanf() is optional and is only selected by CONFIG_XEN so I assumed there would be concerns over binary size increase if USB_HOST would require sscanf.

Good point, lets postpone sscanf() . Also, looking at it, sscanf would 
work for numbers, but not for the special characters. So ... do you want 
to try tweaking this further with strsep() or shall we go with this 
implementation ?