* [PATCH] usb: xhci: Add module param for compliance quirk checking
@ 2024-12-02 3:18 xiehongyu1
2024-12-02 6:38 ` Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: xiehongyu1 @ 2024-12-02 3:18 UTC (permalink / raw)
To: mathias.nyman; +Cc: gregkh, linux-usb, linux-kernel, Hongyu Xie
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.
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");
+
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;
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] usb: xhci: Add module param for compliance quirk checking
2024-12-02 3:18 [PATCH] usb: xhci: Add module param for compliance quirk checking xiehongyu1
@ 2024-12-02 6:38 ` Greg KH
2024-12-03 0:30 ` xiehongyu1
0 siblings, 1 reply; 4+ messages in thread
From: Greg KH @ 2024-12-02 6:38 UTC (permalink / raw)
To: xiehongyu1; +Cc: mathias.nyman, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: xhci: Add module param for compliance quirk checking
2024-12-02 6:38 ` Greg KH
@ 2024-12-03 0:30 ` xiehongyu1
2024-12-04 14:55 ` Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: xiehongyu1 @ 2024-12-03 0:30 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, linux-usb, mathias.nyman, xiehongyu1
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: xhci: Add module param for compliance quirk checking
2024-12-03 0:30 ` xiehongyu1
@ 2024-12-04 14:55 ` Greg KH
0 siblings, 0 replies; 4+ messages in thread
From: Greg KH @ 2024-12-04 14:55 UTC (permalink / raw)
To: xiehongyu1; +Cc: linux-kernel, linux-usb, mathias.nyman
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-12-04 14:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-02 3:18 [PATCH] usb: xhci: Add module param for compliance quirk checking xiehongyu1
2024-12-02 6:38 ` Greg KH
2024-12-03 0:30 ` xiehongyu1
2024-12-04 14:55 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox