Re: [PATCH] usb: cdnsp: remove not used variables

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jan 11, 2022 at 10:38:53AM +0000, Pawel Laszczak wrote:
> >
> >On Tue, Jan 11, 2022 at 09:59:34AM +0100, Pawel Laszczak wrote:
> >> From: Pawel Laszczak <pawell@cadence.com>
> >>
> >> Patch removes not used variables:
> >>  - ret from cdnsp_decode_trb function
> >>  - temp_64 from cdnsp_run function
> >>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Signed-off-by: Pawel Laszczak <pawell@cadence.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

You should check somehow that you do not overflow the buffer, right?  To
not do so is a bit odd.

> >> --- 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

Yes, you are doing two different things here.

thanks,

greg k-h