| Message ID | 20241202031855.1319821-1-xiehongyu1@kylinos.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: xhci: Add module param for compliance quirk checking | expand |
On Mon, Dec 02, 2024 at 11:18:55AM +0800, xiehongyu1@kylinos.cn wrote: > From: Hongyu Xie <xiehongyu1@kylinos.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@kylinos.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 :( thanks, greg k-h
Hi Greg, On 2024/12/2 14:38, Greg KH wrote: > On Mon, Dec 02, 2024 at 11:18:55AM +0800, xiehongyu1@kylinos.cn wrote: >> From: Hongyu Xie <xiehongyu1@kylinos.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@kylinos.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. 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. So I figure using module_param can do the job. Anyway, Is there better way to do this in kernel? > > > thanks, > > greg k-h >
On Tue, Dec 03, 2024 at 08:30:40AM +0800, xiehongyu1@kylinos.cn wrote: > Hi Greg, > > On 2024/12/2 14:38, Greg KH wrote: > > On Mon, Dec 02, 2024 at 11:18:55AM +0800, xiehongyu1@kylinos.cn wrote: > >> From: Hongyu Xie <xiehongyu1@kylinos.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@kylinos.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
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"); + static bool td_on_ring(struct xhci_td *td, struct xhci_ring *ring) { struct xhci_segment *seg; @@ -442,13 +450,19 @@ static bool xhci_compliance_mode_recovery_timer_quirk_check(void) if (!dmi_product_name || !dmi_sys_vendor) return false; - if (!(strstr(dmi_sys_vendor, "Hewlett-Packard"))) + if (!(strstr(dmi_sys_vendor, "Hewlett-Packard")) && !compliance_vendor) + return false; + + if (compliance_vendor && !(strstr(dmi_sys_vendor, + compliance_vendor))) return false; if (strstr(dmi_product_name, "Z420") || strstr(dmi_product_name, "Z620") || strstr(dmi_product_name, "Z820") || - strstr(dmi_product_name, "Z1 Workstation")) + strstr(dmi_product_name, "Z1 Workstation") || + (compliance_product && strstr(dmi_product_name, + compliance_product))) return true; return false;