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@xxxxxxxxxxx>
> >>
> >> Patch removes not used variables:
> >> - ret from cdnsp_decode_trb function
> >> - temp_64 from cdnsp_run function
> >>
> >> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> >> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
> >> ---
> >> 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
