linux-kernel - Re: [PATCH] usb: xhci: Add module param for compliance quirk checking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2024120406-yelp-immunize-ed98@gregkh>
Date: Wed, 4 Dec 2024 15:55:35 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: xiehongyu1@...inos.cn
Cc: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	mathias.nyman@...el.com
Subject: Re: [PATCH] usb: xhci: Add module param for compliance quirk checking

On Tue, Dec 03, 2024 at 08:30:40AM +0800, xiehongyu1@...inos.cn wrote:
> Hi Greg,
> 
> On 2024/12/2 14:38, Greg KH wrote:
> > On Mon, Dec 02, 2024 at 11:18:55AM +0800, xiehongyu1@...inos.cn wrote:
> >> From: Hongyu Xie <xiehongyu1@...inos.cn>
> >>
> >> In the old way, vendor name and product name need to be put in
> >> xhci_compliance_mode_recovery_timer_quirk_check, it's not convenient.
> >>
> >> So add two module param for convenience.
> >
> > Please no.  Module parameters are from the 1990's, they do not scale or
> > work well anymore, please never add them.
> >
> >>
> >> usage: put xhci-hcd.compliance_vendor=[vendor name]
> >> xhci-hcd.compliance_product=[product name] in cmdline.
> >>
> >> In Ubuntu you can use "dmidecode -t system" to get vendor name and
> >> product name.
> >>
> >> Signed-off-by: Hongyu Xie <xiehongyu1@...inos.cn>
> >> ---
> >>  drivers/usb/host/xhci.c | 18 ++++++++++++++++--
> >>  1 file changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> >> index 5ebde8cae4fc..2007c27bfaf4 100644
> >> --- a/drivers/usb/host/xhci.c
> >> +++ b/drivers/usb/host/xhci.c
> >> @@ -39,6 +39,14 @@ static unsigned long long quirks;
> >>  module_param(quirks, ullong, S_IRUGO);
> >>  MODULE_PARM_DESC(quirks, "Bit flags for quirks to be enabled as default");
> >>  
> >> +static char *compliance_product;
> >> +module_param(compliance_product, charp, 0644);
> >> +MODULE_PARM_DESC(compliance_product, "Product name for compliance comparison");
> >> +
> >> +static char *compliance_vendor;
> >> +module_param(compliance_vendor, charp, 0644);
> >> +MODULE_PARM_DESC(compliance_vendor, "Vendor name for compliance comparison");
> >
> > Also, you have provided no documentation as to why these are needed at
> > all, so that's not going to work :(
> Engenieer from RENESA suggested to put vendor name and product name in
> xhci_compliance_mode_recovery_timer_quirk_check for some buggy
> motherboard that using uPD720201.

Why not fix the hardware instead?  And what is this going to "check"?

> For oem(or odm), there might be multiple names for the same
> motherboard(or same design). And put all the names in
> xhci_compliance_mode_recovery_timer_quirk_check might not be a good
> idea.

It should be ok to do that if all of this is broken hardware, right?

> So I figure using module_param can do the job. Anyway, Is there
> better way to do this in kernel?

sysfs attribute?  That way you set it for the specific device you care
about, not for ALL devices in the system which is what this patch does.
Remember, many systems have multiple xhci controllers (I have a laptop
somewhere around here with 4 xhci host controllers.)

thanks,

greg k-h