2011-06-13 21:30:08

by Németh Márton

Subject: [PATCH 1/3] usbip: only dump valid port status

From: Márton Németh <[email protected]>

The wIndex parameter of vhci_hub_control() is always zero when the
request type is GetHubDescriptor, see drivers/usb/core/hub.c::get_hub_descriptor()
and Universal Serial Bus Specification revision 2.0 (April 27, 2000) Section
11.24.2.5 "Get Hub Descriptor". This means the variable rhport will not contain
any useful index. Only use valid rhport values.

Signed-off-by: Márton Németh <[email protected]>
---
drivers/staging/usbip/vhci_hcd.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index 359b464..512a97f 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -464,8 +464,11 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,

if (usbip_dbg_flag_vhci_rh) {
pr_debug("port %d\n", rhport);
- dump_port_status(prev_port_status[rhport]);
- dump_port_status(dum->port_status[rhport]);
+ /* Only dump valid port status */
+ if (0 <= rhport) {
+ dump_port_status(prev_port_status[rhport]);
+ dump_port_status(dum->port_status[rhport]);
+ }
}
usbip_dbg_vhci_rh(" bye\n");

--
1.7.2.5

2011-06-13 21:42:18

by Joe Perches

Subject: Re: [PATCH 1/3] usbip: only dump valid port status

On Mon, 2011-06-13 at 23:29 +0200, Németh Márton wrote:
> From: Márton Németh <[email protected]>
>
> The wIndex parameter of vhci_hub_control() is always zero when the
> request type is GetHubDescriptor,
> ---
> diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
> @@ -464,8 +464,11 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>
> if (usbip_dbg_flag_vhci_rh) {
> pr_debug("port %d\n", rhport);
> - dump_port_status(prev_port_status[rhport]);
> - dump_port_status(dum->port_status[rhport]);
> + /* Only dump valid port status */
> + if (0 <= rhport) {
> + dump_port_status(prev_port_status[rhport]);
> + dump_port_status(dum->port_status[rhport]);
> + }
> }
> usbip_dbg_vhci_rh(" bye\n");
>

Linux relatively rarely uses constant before variable in a comparison.
ie: please use "if (rhport > 0)" instead

2011-06-13 21:47:49

by Németh Márton

Subject: Re: [PATCH 1/3] usbip: only dump valid port status

From: Márton Németh <[email protected]>

The wIndex parameter of vhci_hub_control() is always zero when the
request type is GetHubDescriptor, see drivers/usb/core/hub.c::get_hub_descriptor()
and Universal Serial Bus Specification revision 2.0 (April 27, 2000) Section
11.24.2.5 "Get Hub Descriptor". This means the variable rhport will not contain
any useful index. Only use valid rhport values.

Signed-off-by: Márton Németh <[email protected]>
---
drivers/staging/usbip/vhci_hcd.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index 359b464..512a97f 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -464,8 +464,11 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,

if (usbip_dbg_flag_vhci_rh) {
pr_debug("port %d\n", rhport);
- dump_port_status(prev_port_status[rhport]);
- dump_port_status(dum->port_status[rhport]);
+ /* Only dump valid port status */
+ if (rhport >= 0) {
+ dump_port_status(prev_port_status[rhport]);
+ dump_port_status(dum->port_status[rhport]);
+ }
}
usbip_dbg_vhci_rh(" bye\n");

--
1.7.2.5

2011-06-13 22:23:54

by Joe Perches

Subject: Re: [PATCH 1/3] usbip: only dump valid port status

On Mon, 2011-06-13 at 23:47 +0200, Németh Márton wrote:
> From: Márton Németh <[email protected]>
>
> The wIndex parameter of vhci_hub_control() is always zero when the
> request type is GetHubDescriptor, see drivers/usb/core/hub.c::get_hub_descriptor()
> and Universal Serial Bus Specification revision 2.0 (April 27, 2000) Section
> 11.24.2.5 "Get Hub Descriptor". This means the variable rhport will not contain
> any useful index. Only use valid rhport values.
>
> Signed-off-by: Márton Németh <[email protected]>
> ---
> drivers/staging/usbip/vhci_hcd.c | 7 +++++--
> 1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
> index 359b464..512a97f 100644
> --- a/drivers/staging/usbip/vhci_hcd.c
> +++ b/drivers/staging/usbip/vhci_hcd.c
> @@ -464,8 +464,11 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>
> if (usbip_dbg_flag_vhci_rh) {
> pr_debug("port %d\n", rhport);
> - dump_port_status(prev_port_status[rhport]);
> - dump_port_status(dum->port_status[rhport]);
> + /* Only dump valid port status */
> + if (rhport >= 0) {
> + dump_port_status(prev_port_status[rhport]);
> + dump_port_status(dum->port_status[rhport]);
> + }

Umm, you mean to use "rhport > 0" yes?

2011-06-14 04:55:32

by Németh Márton

Subject: Re: [PATCH 1/3] usbip: only dump valid port status

Joe Perches wrote:
> On Mon, 2011-06-13 at 23:47 +0200, Németh Márton wrote:
>> From: Márton Németh <[email protected]>
>>
>> The wIndex parameter of vhci_hub_control() is always zero when the
>> request type is GetHubDescriptor, see drivers/usb/core/hub.c::get_hub_descriptor()
>> and Universal Serial Bus Specification revision 2.0 (April 27, 2000) Section
>> 11.24.2.5 "Get Hub Descriptor". This means the variable rhport will not contain
>> any useful index. Only use valid rhport values.
>>
>> Signed-off-by: Márton Németh <[email protected]>
>> ---
>> drivers/staging/usbip/vhci_hcd.c | 7 +++++--
>> 1 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
>> index 359b464..512a97f 100644
>> --- a/drivers/staging/usbip/vhci_hcd.c
>> +++ b/drivers/staging/usbip/vhci_hcd.c
>> @@ -464,8 +464,11 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>>
>> if (usbip_dbg_flag_vhci_rh) {
>> pr_debug("port %d\n", rhport);
>> - dump_port_status(prev_port_status[rhport]);
>> - dump_port_status(dum->port_status[rhport]);
>> + /* Only dump valid port status */
>> + if (rhport >= 0) {
>> + dump_port_status(prev_port_status[rhport]);
>> + dump_port_status(dum->port_status[rhport]);
>> + }
>
> Umm, you mean to use "rhport > 0" yes?

No, it is really (rhport >= 0). The wIndex parameter is zero, the
rhport is wIndex - 1. So rhport zero is valid, rhport -1 is not valid.

Regards,

Márton Németh