From: Pawel Laszczak <[email protected]>
Patch removes not used variables:
- ret from cdnsp_decode_trb function
- temp_64 from cdnsp_run function
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Pawel Laszczak <[email protected]>
---
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;
u32 temp;
ep_num = DIV_ROUND_UP(ep_id, 2);
switch (type) {
case TRB_LINK:
- ret += snprintf(str, size,
- "LINK %08x%08x intr %ld type '%s' flags %c:%c:%c:%c",
- field1, field0, GET_INTR_TARGET(field2),
- cdnsp_trb_type_string(type),
- field3 & TRB_IOC ? 'I' : 'i',
- field3 & TRB_CHAIN ? 'C' : 'c',
- field3 & TRB_TC ? 'T' : 't',
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size,
+ "LINK %08x%08x intr %ld type '%s' flags %c:%c:%c:%c",
+ field1, field0, GET_INTR_TARGET(field2),
+ cdnsp_trb_type_string(type),
+ field3 & TRB_IOC ? 'I' : 'i',
+ field3 & TRB_CHAIN ? 'C' : 'c',
+ field3 & TRB_TC ? 'T' : 't',
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_TRANSFER:
case TRB_COMPLETION:
case TRB_PORT_STATUS:
case TRB_HC_EVENT:
- ret += snprintf(str, size,
- "ep%d%s(%d) type '%s' TRB %08x%08x status '%s'"
- " len %ld slot %ld flags %c:%c",
- ep_num, ep_id % 2 ? "out" : "in",
- TRB_TO_EP_INDEX(field3),
- cdnsp_trb_type_string(type), field1, field0,
- cdnsp_trb_comp_code_string(GET_COMP_CODE(field2)),
- EVENT_TRB_LEN(field2), TRB_TO_SLOT_ID(field3),
- field3 & EVENT_DATA ? 'E' : 'e',
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "ep%d%s(%d) type '%s' TRB %08x%08x"
+ " status '%s' len %ld slot %ld flags %c:%c",
+ ep_num, ep_id % 2 ? "out" : "in",
+ TRB_TO_EP_INDEX(field3),
+ cdnsp_trb_type_string(type), field1, field0,
+ cdnsp_trb_comp_code_string(GET_COMP_CODE(field2)),
+ EVENT_TRB_LEN(field2), TRB_TO_SLOT_ID(field3),
+ field3 & EVENT_DATA ? 'E' : 'e',
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_MFINDEX_WRAP:
- ret += snprintf(str, size, "%s: flags %c",
- cdnsp_trb_type_string(type),
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "%s: flags %c",
+ cdnsp_trb_type_string(type),
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_SETUP:
- ret += snprintf(str, size,
- "type '%s' bRequestType %02x bRequest %02x "
- "wValue %02x%02x wIndex %02x%02x wLength %d "
- "length %ld TD size %ld intr %ld Setup ID %ld "
- "flags %c:%c:%c",
- cdnsp_trb_type_string(type),
- field0 & 0xff,
- (field0 & 0xff00) >> 8,
- (field0 & 0xff000000) >> 24,
- (field0 & 0xff0000) >> 16,
- (field1 & 0xff00) >> 8,
- field1 & 0xff,
- (field1 & 0xff000000) >> 16 |
- (field1 & 0xff0000) >> 16,
- TRB_LEN(field2), GET_TD_SIZE(field2),
- GET_INTR_TARGET(field2),
- TRB_SETUPID_TO_TYPE(field3),
- field3 & TRB_IDT ? 'D' : 'd',
- field3 & TRB_IOC ? 'I' : 'i',
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "type '%s' bRequestType %02x "
+ "bRequest %02x wValue %02x%02x wIndex %02x%02x "
+ "wLength %d length %ld TD size %ld intr %ld "
+ "Setup ID %ld flags %c:%c:%c",
+ cdnsp_trb_type_string(type),
+ field0 & 0xff,
+ (field0 & 0xff00) >> 8,
+ (field0 & 0xff000000) >> 24,
+ (field0 & 0xff0000) >> 16,
+ (field1 & 0xff00) >> 8,
+ field1 & 0xff,
+ (field1 & 0xff000000) >> 16 |
+ (field1 & 0xff0000) >> 16,
+ TRB_LEN(field2), GET_TD_SIZE(field2),
+ GET_INTR_TARGET(field2),
+ TRB_SETUPID_TO_TYPE(field3),
+ field3 & TRB_IDT ? 'D' : 'd',
+ field3 & TRB_IOC ? 'I' : 'i',
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_DATA:
- ret += snprintf(str, size,
- "type '%s' Buffer %08x%08x length %ld TD size %ld "
- "intr %ld flags %c:%c:%c:%c:%c:%c:%c",
- cdnsp_trb_type_string(type),
- field1, field0, TRB_LEN(field2),
- GET_TD_SIZE(field2),
- GET_INTR_TARGET(field2),
- field3 & TRB_IDT ? 'D' : 'i',
- field3 & TRB_IOC ? 'I' : 'i',
- field3 & TRB_CHAIN ? 'C' : 'c',
- field3 & TRB_NO_SNOOP ? 'S' : 's',
- field3 & TRB_ISP ? 'I' : 'i',
- field3 & TRB_ENT ? 'E' : 'e',
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "type '%s' Buffer %08x%08x length %ld "
+ "TD size %ld intr %ld flags %c:%c:%c:%c:%c:%c:%c",
+ cdnsp_trb_type_string(type),
+ field1, field0, TRB_LEN(field2),
+ GET_TD_SIZE(field2),
+ GET_INTR_TARGET(field2),
+ field3 & TRB_IDT ? 'D' : 'i',
+ field3 & TRB_IOC ? 'I' : 'i',
+ field3 & TRB_CHAIN ? 'C' : 'c',
+ field3 & TRB_NO_SNOOP ? 'S' : 's',
+ field3 & TRB_ISP ? 'I' : 'i',
+ field3 & TRB_ENT ? 'E' : 'e',
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_STATUS:
- ret += snprintf(str, size,
- "Buffer %08x%08x length %ld TD size %ld intr"
- "%ld type '%s' flags %c:%c:%c:%c",
- field1, field0, TRB_LEN(field2),
- GET_TD_SIZE(field2),
- GET_INTR_TARGET(field2),
- cdnsp_trb_type_string(type),
- field3 & TRB_IOC ? 'I' : 'i',
- field3 & TRB_CHAIN ? 'C' : 'c',
- field3 & TRB_ENT ? 'E' : 'e',
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "Buffer %08x%08x length %ld TD size %ld"
+ " intr %ld type '%s' flags %c:%c:%c:%c",
+ field1, field0, TRB_LEN(field2),
+ GET_TD_SIZE(field2),
+ GET_INTR_TARGET(field2),
+ cdnsp_trb_type_string(type),
+ field3 & TRB_IOC ? 'I' : 'i',
+ field3 & TRB_CHAIN ? 'C' : 'c',
+ field3 & TRB_ENT ? 'E' : 'e',
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_NORMAL:
case TRB_ISOC:
case TRB_EVENT_DATA:
case TRB_TR_NOOP:
- ret += snprintf(str, size,
- "type '%s' Buffer %08x%08x length %ld "
- "TD size %ld intr %ld "
- "flags %c:%c:%c:%c:%c:%c:%c:%c:%c",
- cdnsp_trb_type_string(type),
- field1, field0, TRB_LEN(field2),
- GET_TD_SIZE(field2),
- GET_INTR_TARGET(field2),
- field3 & TRB_BEI ? 'B' : 'b',
- field3 & TRB_IDT ? 'T' : 't',
- field3 & TRB_IOC ? 'I' : 'i',
- field3 & TRB_CHAIN ? 'C' : 'c',
- field3 & TRB_NO_SNOOP ? 'S' : 's',
- field3 & TRB_ISP ? 'I' : 'i',
- field3 & TRB_ENT ? 'E' : 'e',
- field3 & TRB_CYCLE ? 'C' : 'c',
- !(field3 & TRB_EVENT_INVALIDATE) ? 'V' : 'v');
+ snprintf(str, size, "type '%s' Buffer %08x%08x length %ld "
+ "TD size %ld intr %ld "
+ "flags %c:%c:%c:%c:%c:%c:%c:%c:%c",
+ cdnsp_trb_type_string(type),
+ field1, field0, TRB_LEN(field2),
+ GET_TD_SIZE(field2),
+ GET_INTR_TARGET(field2),
+ field3 & TRB_BEI ? 'B' : 'b',
+ field3 & TRB_IDT ? 'T' : 't',
+ field3 & TRB_IOC ? 'I' : 'i',
+ field3 & TRB_CHAIN ? 'C' : 'c',
+ field3 & TRB_NO_SNOOP ? 'S' : 's',
+ field3 & TRB_ISP ? 'I' : 'i',
+ field3 & TRB_ENT ? 'E' : 'e',
+ field3 & TRB_CYCLE ? 'C' : 'c',
+ !(field3 & TRB_EVENT_INVALIDATE) ? 'V' : 'v');
break;
case TRB_CMD_NOOP:
case TRB_ENABLE_SLOT:
- ret += snprintf(str, size, "%s: flags %c",
- cdnsp_trb_type_string(type),
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "%s: flags %c",
+ cdnsp_trb_type_string(type),
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_DISABLE_SLOT:
- ret += snprintf(str, size, "%s: slot %ld flags %c",
- cdnsp_trb_type_string(type),
- TRB_TO_SLOT_ID(field3),
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "%s: slot %ld flags %c",
+ cdnsp_trb_type_string(type),
+ TRB_TO_SLOT_ID(field3),
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_ADDR_DEV:
- ret += snprintf(str, size,
- "%s: ctx %08x%08x slot %ld flags %c:%c",
- cdnsp_trb_type_string(type), field1, field0,
- TRB_TO_SLOT_ID(field3),
- field3 & TRB_BSR ? 'B' : 'b',
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "%s: ctx %08x%08x slot %ld flags %c:%c",
+ cdnsp_trb_type_string(type), field1, field0,
+ TRB_TO_SLOT_ID(field3),
+ field3 & TRB_BSR ? 'B' : 'b',
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_CONFIG_EP:
- ret += snprintf(str, size,
- "%s: ctx %08x%08x slot %ld flags %c:%c",
- cdnsp_trb_type_string(type), field1, field0,
- TRB_TO_SLOT_ID(field3),
- field3 & TRB_DC ? 'D' : 'd',
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "%s: ctx %08x%08x slot %ld flags %c:%c",
+ cdnsp_trb_type_string(type), field1, field0,
+ TRB_TO_SLOT_ID(field3),
+ field3 & TRB_DC ? 'D' : 'd',
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_EVAL_CONTEXT:
- ret += snprintf(str, size,
- "%s: ctx %08x%08x slot %ld flags %c",
- cdnsp_trb_type_string(type), field1, field0,
- TRB_TO_SLOT_ID(field3),
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "%s: ctx %08x%08x slot %ld flags %c",
+ cdnsp_trb_type_string(type), field1, field0,
+ TRB_TO_SLOT_ID(field3),
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_RESET_EP:
case TRB_HALT_ENDPOINT:
case TRB_FLUSH_ENDPOINT:
- ret += snprintf(str, size,
- "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags %c",
- cdnsp_trb_type_string(type),
- ep_num, ep_id % 2 ? "out" : "in",
- TRB_TO_EP_INDEX(field3), field1, field0,
- TRB_TO_SLOT_ID(field3),
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size,
+ "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags %c",
+ cdnsp_trb_type_string(type),
+ ep_num, ep_id % 2 ? "out" : "in",
+ TRB_TO_EP_INDEX(field3), field1, field0,
+ TRB_TO_SLOT_ID(field3),
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_STOP_RING:
- ret += snprintf(str, size,
- "%s: ep%d%s(%d) slot %ld sp %d flags %c",
- cdnsp_trb_type_string(type),
- ep_num, ep_id % 2 ? "out" : "in",
- TRB_TO_EP_INDEX(field3),
- TRB_TO_SLOT_ID(field3),
- TRB_TO_SUSPEND_PORT(field3),
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "%s: ep%d%s(%d) slot %ld sp %d flags %c",
+ cdnsp_trb_type_string(type),
+ ep_num, ep_id % 2 ? "out" : "in",
+ TRB_TO_EP_INDEX(field3),
+ TRB_TO_SLOT_ID(field3),
+ TRB_TO_SUSPEND_PORT(field3),
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_SET_DEQ:
- ret += snprintf(str, size,
- "%s: ep%d%s(%d) deq %08x%08x stream %ld slot %ld flags %c",
- cdnsp_trb_type_string(type),
- ep_num, ep_id % 2 ? "out" : "in",
- TRB_TO_EP_INDEX(field3), field1, field0,
- TRB_TO_STREAM_ID(field2),
- TRB_TO_SLOT_ID(field3),
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "%s: ep%d%s(%d) deq %08x%08x stream %ld"
+ " slot %ld flags %c",
+ cdnsp_trb_type_string(type),
+ ep_num, ep_id % 2 ? "out" : "in",
+ TRB_TO_EP_INDEX(field3), field1, field0,
+ TRB_TO_STREAM_ID(field2),
+ TRB_TO_SLOT_ID(field3),
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_RESET_DEV:
- ret += snprintf(str, size, "%s: slot %ld flags %c",
- cdnsp_trb_type_string(type),
- TRB_TO_SLOT_ID(field3),
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size, "%s: slot %ld flags %c",
+ cdnsp_trb_type_string(type),
+ TRB_TO_SLOT_ID(field3),
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
case TRB_ENDPOINT_NRDY:
temp = TRB_TO_HOST_STREAM(field2);
- ret += snprintf(str, size,
- "%s: ep%d%s(%d) H_SID %x%s%s D_SID %lx flags %c:%c",
- cdnsp_trb_type_string(type),
- ep_num, ep_id % 2 ? "out" : "in",
- TRB_TO_EP_INDEX(field3), temp,
- temp == STREAM_PRIME_ACK ? "(PRIME)" : "",
- temp == STREAM_REJECTED ? "(REJECTED)" : "",
- TRB_TO_DEV_STREAM(field0),
- field3 & TRB_STAT ? 'S' : 's',
- field3 & TRB_CYCLE ? 'C' : 'c');
+ snprintf(str, size,
+ "%s: ep%d%s(%d) H_SID %x%s%s D_SID %lx flags %c:%c",
+ cdnsp_trb_type_string(type),
+ ep_num, ep_id % 2 ? "out" : "in",
+ TRB_TO_EP_INDEX(field3), temp,
+ temp == STREAM_PRIME_ACK ? "(PRIME)" : "",
+ temp == STREAM_REJECTED ? "(REJECTED)" : "",
+ TRB_TO_DEV_STREAM(field0),
+ field3 & TRB_STAT ? 'S' : 's',
+ field3 & TRB_CYCLE ? 'C' : 'c');
break;
default:
- ret += snprintf(str, size,
- "type '%s' -> raw %08x %08x %08x %08x",
- cdnsp_trb_type_string(type),
- field0, field1, field2, field3);
+ snprintf(str, size, "type '%s' -> raw %08x %08x %08x %08x",
+ cdnsp_trb_type_string(type),
+ field0, field1, field2, field3);
}
return str;
diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
index 27df0c697897..ec8a0dc792bc 100644
--- 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
On Tue, Jan 11, 2022 at 09:59:34AM +0100, Pawel Laszczak wrote:
> From: Pawel Laszczak <[email protected]>
>
> Patch removes not used variables:
> - ret from cdnsp_decode_trb function
> - temp_64 from cdnsp_run function
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Pawel Laszczak <[email protected]>
> ---
> 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?
> --- 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.
thanks,
greg k-h
>
>On Tue, Jan 11, 2022 at 09:59:34AM +0100, Pawel Laszczak wrote:
>> From: Pawel Laszczak <[email protected]>
>>
>> Patch removes not used variables:
>> - ret from cdnsp_decode_trb function
>> - temp_64 from cdnsp_run function
>>
>> Reported-by: kernel test robot <[email protected]>
>> Signed-off-by: Pawel Laszczak <[email protected]>
>> ---
>> 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
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 <[email protected]>
> >>
> >> Patch removes not used variables:
> >> - ret from cdnsp_decode_trb function
> >> - temp_64 from cdnsp_run function
> >>
> >> Reported-by: kernel test robot <[email protected]>
> >> Signed-off-by: Pawel Laszczak <[email protected]>
> >> ---
> >> 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
On 22-01-11 09:59:34, Pawel Laszczak wrote:
> From: Pawel Laszczak <[email protected]>
>
> Patch removes not used variables:
> - ret from cdnsp_decode_trb function
> - temp_64 from cdnsp_run function
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Pawel Laszczak <[email protected]>
Reviewed-by: Peter Chen <[email protected]>
Peter
> ---
> 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;
> u32 temp;
>
> ep_num = DIV_ROUND_UP(ep_id, 2);
>
> switch (type) {
> case TRB_LINK:
> - ret += snprintf(str, size,
> - "LINK %08x%08x intr %ld type '%s' flags %c:%c:%c:%c",
> - field1, field0, GET_INTR_TARGET(field2),
> - cdnsp_trb_type_string(type),
> - field3 & TRB_IOC ? 'I' : 'i',
> - field3 & TRB_CHAIN ? 'C' : 'c',
> - field3 & TRB_TC ? 'T' : 't',
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size,
> + "LINK %08x%08x intr %ld type '%s' flags %c:%c:%c:%c",
> + field1, field0, GET_INTR_TARGET(field2),
> + cdnsp_trb_type_string(type),
> + field3 & TRB_IOC ? 'I' : 'i',
> + field3 & TRB_CHAIN ? 'C' : 'c',
> + field3 & TRB_TC ? 'T' : 't',
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_TRANSFER:
> case TRB_COMPLETION:
> case TRB_PORT_STATUS:
> case TRB_HC_EVENT:
> - ret += snprintf(str, size,
> - "ep%d%s(%d) type '%s' TRB %08x%08x status '%s'"
> - " len %ld slot %ld flags %c:%c",
> - ep_num, ep_id % 2 ? "out" : "in",
> - TRB_TO_EP_INDEX(field3),
> - cdnsp_trb_type_string(type), field1, field0,
> - cdnsp_trb_comp_code_string(GET_COMP_CODE(field2)),
> - EVENT_TRB_LEN(field2), TRB_TO_SLOT_ID(field3),
> - field3 & EVENT_DATA ? 'E' : 'e',
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "ep%d%s(%d) type '%s' TRB %08x%08x"
> + " status '%s' len %ld slot %ld flags %c:%c",
> + ep_num, ep_id % 2 ? "out" : "in",
> + TRB_TO_EP_INDEX(field3),
> + cdnsp_trb_type_string(type), field1, field0,
> + cdnsp_trb_comp_code_string(GET_COMP_CODE(field2)),
> + EVENT_TRB_LEN(field2), TRB_TO_SLOT_ID(field3),
> + field3 & EVENT_DATA ? 'E' : 'e',
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_MFINDEX_WRAP:
> - ret += snprintf(str, size, "%s: flags %c",
> - cdnsp_trb_type_string(type),
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "%s: flags %c",
> + cdnsp_trb_type_string(type),
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_SETUP:
> - ret += snprintf(str, size,
> - "type '%s' bRequestType %02x bRequest %02x "
> - "wValue %02x%02x wIndex %02x%02x wLength %d "
> - "length %ld TD size %ld intr %ld Setup ID %ld "
> - "flags %c:%c:%c",
> - cdnsp_trb_type_string(type),
> - field0 & 0xff,
> - (field0 & 0xff00) >> 8,
> - (field0 & 0xff000000) >> 24,
> - (field0 & 0xff0000) >> 16,
> - (field1 & 0xff00) >> 8,
> - field1 & 0xff,
> - (field1 & 0xff000000) >> 16 |
> - (field1 & 0xff0000) >> 16,
> - TRB_LEN(field2), GET_TD_SIZE(field2),
> - GET_INTR_TARGET(field2),
> - TRB_SETUPID_TO_TYPE(field3),
> - field3 & TRB_IDT ? 'D' : 'd',
> - field3 & TRB_IOC ? 'I' : 'i',
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "type '%s' bRequestType %02x "
> + "bRequest %02x wValue %02x%02x wIndex %02x%02x "
> + "wLength %d length %ld TD size %ld intr %ld "
> + "Setup ID %ld flags %c:%c:%c",
> + cdnsp_trb_type_string(type),
> + field0 & 0xff,
> + (field0 & 0xff00) >> 8,
> + (field0 & 0xff000000) >> 24,
> + (field0 & 0xff0000) >> 16,
> + (field1 & 0xff00) >> 8,
> + field1 & 0xff,
> + (field1 & 0xff000000) >> 16 |
> + (field1 & 0xff0000) >> 16,
> + TRB_LEN(field2), GET_TD_SIZE(field2),
> + GET_INTR_TARGET(field2),
> + TRB_SETUPID_TO_TYPE(field3),
> + field3 & TRB_IDT ? 'D' : 'd',
> + field3 & TRB_IOC ? 'I' : 'i',
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_DATA:
> - ret += snprintf(str, size,
> - "type '%s' Buffer %08x%08x length %ld TD size %ld "
> - "intr %ld flags %c:%c:%c:%c:%c:%c:%c",
> - cdnsp_trb_type_string(type),
> - field1, field0, TRB_LEN(field2),
> - GET_TD_SIZE(field2),
> - GET_INTR_TARGET(field2),
> - field3 & TRB_IDT ? 'D' : 'i',
> - field3 & TRB_IOC ? 'I' : 'i',
> - field3 & TRB_CHAIN ? 'C' : 'c',
> - field3 & TRB_NO_SNOOP ? 'S' : 's',
> - field3 & TRB_ISP ? 'I' : 'i',
> - field3 & TRB_ENT ? 'E' : 'e',
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "type '%s' Buffer %08x%08x length %ld "
> + "TD size %ld intr %ld flags %c:%c:%c:%c:%c:%c:%c",
> + cdnsp_trb_type_string(type),
> + field1, field0, TRB_LEN(field2),
> + GET_TD_SIZE(field2),
> + GET_INTR_TARGET(field2),
> + field3 & TRB_IDT ? 'D' : 'i',
> + field3 & TRB_IOC ? 'I' : 'i',
> + field3 & TRB_CHAIN ? 'C' : 'c',
> + field3 & TRB_NO_SNOOP ? 'S' : 's',
> + field3 & TRB_ISP ? 'I' : 'i',
> + field3 & TRB_ENT ? 'E' : 'e',
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_STATUS:
> - ret += snprintf(str, size,
> - "Buffer %08x%08x length %ld TD size %ld intr"
> - "%ld type '%s' flags %c:%c:%c:%c",
> - field1, field0, TRB_LEN(field2),
> - GET_TD_SIZE(field2),
> - GET_INTR_TARGET(field2),
> - cdnsp_trb_type_string(type),
> - field3 & TRB_IOC ? 'I' : 'i',
> - field3 & TRB_CHAIN ? 'C' : 'c',
> - field3 & TRB_ENT ? 'E' : 'e',
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "Buffer %08x%08x length %ld TD size %ld"
> + " intr %ld type '%s' flags %c:%c:%c:%c",
> + field1, field0, TRB_LEN(field2),
> + GET_TD_SIZE(field2),
> + GET_INTR_TARGET(field2),
> + cdnsp_trb_type_string(type),
> + field3 & TRB_IOC ? 'I' : 'i',
> + field3 & TRB_CHAIN ? 'C' : 'c',
> + field3 & TRB_ENT ? 'E' : 'e',
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_NORMAL:
> case TRB_ISOC:
> case TRB_EVENT_DATA:
> case TRB_TR_NOOP:
> - ret += snprintf(str, size,
> - "type '%s' Buffer %08x%08x length %ld "
> - "TD size %ld intr %ld "
> - "flags %c:%c:%c:%c:%c:%c:%c:%c:%c",
> - cdnsp_trb_type_string(type),
> - field1, field0, TRB_LEN(field2),
> - GET_TD_SIZE(field2),
> - GET_INTR_TARGET(field2),
> - field3 & TRB_BEI ? 'B' : 'b',
> - field3 & TRB_IDT ? 'T' : 't',
> - field3 & TRB_IOC ? 'I' : 'i',
> - field3 & TRB_CHAIN ? 'C' : 'c',
> - field3 & TRB_NO_SNOOP ? 'S' : 's',
> - field3 & TRB_ISP ? 'I' : 'i',
> - field3 & TRB_ENT ? 'E' : 'e',
> - field3 & TRB_CYCLE ? 'C' : 'c',
> - !(field3 & TRB_EVENT_INVALIDATE) ? 'V' : 'v');
> + snprintf(str, size, "type '%s' Buffer %08x%08x length %ld "
> + "TD size %ld intr %ld "
> + "flags %c:%c:%c:%c:%c:%c:%c:%c:%c",
> + cdnsp_trb_type_string(type),
> + field1, field0, TRB_LEN(field2),
> + GET_TD_SIZE(field2),
> + GET_INTR_TARGET(field2),
> + field3 & TRB_BEI ? 'B' : 'b',
> + field3 & TRB_IDT ? 'T' : 't',
> + field3 & TRB_IOC ? 'I' : 'i',
> + field3 & TRB_CHAIN ? 'C' : 'c',
> + field3 & TRB_NO_SNOOP ? 'S' : 's',
> + field3 & TRB_ISP ? 'I' : 'i',
> + field3 & TRB_ENT ? 'E' : 'e',
> + field3 & TRB_CYCLE ? 'C' : 'c',
> + !(field3 & TRB_EVENT_INVALIDATE) ? 'V' : 'v');
> break;
> case TRB_CMD_NOOP:
> case TRB_ENABLE_SLOT:
> - ret += snprintf(str, size, "%s: flags %c",
> - cdnsp_trb_type_string(type),
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "%s: flags %c",
> + cdnsp_trb_type_string(type),
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_DISABLE_SLOT:
> - ret += snprintf(str, size, "%s: slot %ld flags %c",
> - cdnsp_trb_type_string(type),
> - TRB_TO_SLOT_ID(field3),
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "%s: slot %ld flags %c",
> + cdnsp_trb_type_string(type),
> + TRB_TO_SLOT_ID(field3),
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_ADDR_DEV:
> - ret += snprintf(str, size,
> - "%s: ctx %08x%08x slot %ld flags %c:%c",
> - cdnsp_trb_type_string(type), field1, field0,
> - TRB_TO_SLOT_ID(field3),
> - field3 & TRB_BSR ? 'B' : 'b',
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "%s: ctx %08x%08x slot %ld flags %c:%c",
> + cdnsp_trb_type_string(type), field1, field0,
> + TRB_TO_SLOT_ID(field3),
> + field3 & TRB_BSR ? 'B' : 'b',
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_CONFIG_EP:
> - ret += snprintf(str, size,
> - "%s: ctx %08x%08x slot %ld flags %c:%c",
> - cdnsp_trb_type_string(type), field1, field0,
> - TRB_TO_SLOT_ID(field3),
> - field3 & TRB_DC ? 'D' : 'd',
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "%s: ctx %08x%08x slot %ld flags %c:%c",
> + cdnsp_trb_type_string(type), field1, field0,
> + TRB_TO_SLOT_ID(field3),
> + field3 & TRB_DC ? 'D' : 'd',
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_EVAL_CONTEXT:
> - ret += snprintf(str, size,
> - "%s: ctx %08x%08x slot %ld flags %c",
> - cdnsp_trb_type_string(type), field1, field0,
> - TRB_TO_SLOT_ID(field3),
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "%s: ctx %08x%08x slot %ld flags %c",
> + cdnsp_trb_type_string(type), field1, field0,
> + TRB_TO_SLOT_ID(field3),
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_RESET_EP:
> case TRB_HALT_ENDPOINT:
> case TRB_FLUSH_ENDPOINT:
> - ret += snprintf(str, size,
> - "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags %c",
> - cdnsp_trb_type_string(type),
> - ep_num, ep_id % 2 ? "out" : "in",
> - TRB_TO_EP_INDEX(field3), field1, field0,
> - TRB_TO_SLOT_ID(field3),
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size,
> + "%s: ep%d%s(%d) ctx %08x%08x slot %ld flags %c",
> + cdnsp_trb_type_string(type),
> + ep_num, ep_id % 2 ? "out" : "in",
> + TRB_TO_EP_INDEX(field3), field1, field0,
> + TRB_TO_SLOT_ID(field3),
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_STOP_RING:
> - ret += snprintf(str, size,
> - "%s: ep%d%s(%d) slot %ld sp %d flags %c",
> - cdnsp_trb_type_string(type),
> - ep_num, ep_id % 2 ? "out" : "in",
> - TRB_TO_EP_INDEX(field3),
> - TRB_TO_SLOT_ID(field3),
> - TRB_TO_SUSPEND_PORT(field3),
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "%s: ep%d%s(%d) slot %ld sp %d flags %c",
> + cdnsp_trb_type_string(type),
> + ep_num, ep_id % 2 ? "out" : "in",
> + TRB_TO_EP_INDEX(field3),
> + TRB_TO_SLOT_ID(field3),
> + TRB_TO_SUSPEND_PORT(field3),
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_SET_DEQ:
> - ret += snprintf(str, size,
> - "%s: ep%d%s(%d) deq %08x%08x stream %ld slot %ld flags %c",
> - cdnsp_trb_type_string(type),
> - ep_num, ep_id % 2 ? "out" : "in",
> - TRB_TO_EP_INDEX(field3), field1, field0,
> - TRB_TO_STREAM_ID(field2),
> - TRB_TO_SLOT_ID(field3),
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "%s: ep%d%s(%d) deq %08x%08x stream %ld"
> + " slot %ld flags %c",
> + cdnsp_trb_type_string(type),
> + ep_num, ep_id % 2 ? "out" : "in",
> + TRB_TO_EP_INDEX(field3), field1, field0,
> + TRB_TO_STREAM_ID(field2),
> + TRB_TO_SLOT_ID(field3),
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_RESET_DEV:
> - ret += snprintf(str, size, "%s: slot %ld flags %c",
> - cdnsp_trb_type_string(type),
> - TRB_TO_SLOT_ID(field3),
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size, "%s: slot %ld flags %c",
> + cdnsp_trb_type_string(type),
> + TRB_TO_SLOT_ID(field3),
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> case TRB_ENDPOINT_NRDY:
> temp = TRB_TO_HOST_STREAM(field2);
>
> - ret += snprintf(str, size,
> - "%s: ep%d%s(%d) H_SID %x%s%s D_SID %lx flags %c:%c",
> - cdnsp_trb_type_string(type),
> - ep_num, ep_id % 2 ? "out" : "in",
> - TRB_TO_EP_INDEX(field3), temp,
> - temp == STREAM_PRIME_ACK ? "(PRIME)" : "",
> - temp == STREAM_REJECTED ? "(REJECTED)" : "",
> - TRB_TO_DEV_STREAM(field0),
> - field3 & TRB_STAT ? 'S' : 's',
> - field3 & TRB_CYCLE ? 'C' : 'c');
> + snprintf(str, size,
> + "%s: ep%d%s(%d) H_SID %x%s%s D_SID %lx flags %c:%c",
> + cdnsp_trb_type_string(type),
> + ep_num, ep_id % 2 ? "out" : "in",
> + TRB_TO_EP_INDEX(field3), temp,
> + temp == STREAM_PRIME_ACK ? "(PRIME)" : "",
> + temp == STREAM_REJECTED ? "(REJECTED)" : "",
> + TRB_TO_DEV_STREAM(field0),
> + field3 & TRB_STAT ? 'S' : 's',
> + field3 & TRB_CYCLE ? 'C' : 'c');
> break;
> default:
> - ret += snprintf(str, size,
> - "type '%s' -> raw %08x %08x %08x %08x",
> - cdnsp_trb_type_string(type),
> - field0, field1, field2, field3);
> + snprintf(str, size, "type '%s' -> raw %08x %08x %08x %08x",
> + cdnsp_trb_type_string(type),
> + field0, field1, field2, field3);
> }
>
> return str;
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
> index 27df0c697897..ec8a0dc792bc 100644
> --- 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
>
--
Thanks,
Peter Chen