linux-kernel - RE: [PATCH] usb: cdnsp: remove not used variables

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <BYAPR07MB53811E109250D3D9C6447847DD519@BYAPR07MB5381.namprd07.prod.outlook.com>
Date:   Tue, 11 Jan 2022 10:38:53 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "peter.chen@...nel.org" <peter.chen@...nel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] usb: cdnsp: remove not used variables

>
>On Tue, Jan 11, 2022 at 09:59:34AM +0100, Pawel Laszczak wrote:
>> From: Pawel Laszczak <pawell@...ence.com>
>>
>> Patch removes not used variables:
>>  - ret from cdnsp_decode_trb function
>>  - temp_64 from cdnsp_run function
>>
>> Reported-by: kernel test robot <lkp@...el.com>
>> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
>> ---
>>  drivers/usb/cdns3/cdnsp-debug.h  | 287 +++++++++++++++----------------
>>  drivers/usb/cdns3/cdnsp-gadget.c |   3 -
>>  2 files changed, 138 insertions(+), 152 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-debug.h b/drivers/usb/cdns3/cdnsp-debug.h
>> index a8776df2d4e0..29f3cf7ddbaa 100644
>> --- a/drivers/usb/cdns3/cdnsp-debug.h
>> +++ b/drivers/usb/cdns3/cdnsp-debug.h
>> @@ -182,206 +182,195 @@ static inline const char *cdnsp_decode_trb(char *str, size_t size, u32 field0,
>>  	int ep_id = TRB_TO_EP_INDEX(field3) - 1;
>>  	int type = TRB_FIELD_TO_TYPE(field3);
>>  	unsigned int ep_num;
>> -	int ret = 0;
>
>Please fix this function to properly handle the ret value, as I think it
>should be checked, right?

I think that it is not needed. Function is used only in one place in trace point in TP_printk. The buffer is
big enough (500 bytes) to accommodate whole string.  In this form function can be directly used in
TP_printk. If we will use ret instead of string as return type, then driver will need to format this string before
calling trace point function and pass this ass parameter.  This solution will have impact for code size and
performance even if we disable tracing

>
>> --- a/drivers/usb/cdns3/cdnsp-gadget.c
>> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
>> @@ -1243,12 +1243,9 @@ static int cdnsp_run(struct cdnsp_device *pdev,
>>  		     enum usb_device_speed speed)
>>  {
>>  	u32 fs_speed = 0;
>> -	u64 temp_64;
>>  	u32 temp;
>>  	int ret;
>>
>> -	temp_64 = cdnsp_read_64(&pdev->ir_set->erst_dequeue);
>> -	temp_64 &= ~ERST_PTR_MASK;
>>  	temp = readl(&pdev->ir_set->irq_control);
>>  	temp &= ~IMOD_INTERVAL_MASK;
>>  	temp |= ((IMOD_DEFAULT_INTERVAL / 250) & IMOD_INTERVAL_MASK);
>> --
>> 2.25.1
>>
>
>A separate patch for this.
>
>Also, are you SURE this is ok to do?  Did you check it on the hardware
>that a read is not needed here for it to work properly?
>
>This type of "warning" is horrible for dealing with hardware devices,
>always treat it as incorrect unless you can prove otherwise.
>

Yes, I've tested it. I think that it was used in some printk and by mistake has not been removed. 

The warning was reported by Intel kernel test robot and fix are very simple. Patch is little bigger
because some code had to be reformatted.  

Do I really need to send this as two separate patches

thanks,

Pawel Laszczak