drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state() e

PATCH In review Fengguang Wu 5 participants 6 messages Sep 12, 2012 1 message not in the archive Kernel janitors

lore

Share & tools

lore

Fengguang Wu

author

2012-09-12 08:26 UTC #0 lore raw

PATCH 66/99 drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state() e

Hi Felipe,

FYI, there are new smatch warnings show up in

tree:   git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git merge-result-for-greg
head:   23953bde3e4d6aa8780dc054f6ad9882ac63f4f4
commit: e918fa161f510136fce45a524e934fe20e62c8b1 [66/99] Merge tag 'gadget-for-v3.7' into merge-result-for-greg

drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state() error: doing dma on the stack ((null)) drivers/usb/core/hub.c:4733 store_port_power_control() info: why not propagate 'ret' from set_port_feature() instead of -5? drivers/usb/core/hub.c:4744 store_port_power_control() info: why not propagate 'ret' from clear_port_feature() instead of -5?

vim +2654 drivers/usb/core/hub.c
2644 return ret;
2645 }
2646
2647 static int usb_get_hub_port_power_state(struct usb_device *hdev, int port1)
2648 {
2649 struct usb_hub *hub = hdev_to_hub(hdev);
2650 struct usb_port_status data;
2651 u16 portstatus;
2652 int ret;
2653

Quoted 1 line

2654 ret = get_port_status(hub->hdev, port1, &data);

2655 if (ret < 4) {
2656 dev_err(hub->intfdev,
2657 "%s failed (err = %d)\n", __func__, ret);
2658 if (ret >= 0)
2659 ret = -EIO;
2660 return ret;
2661 } else
2662 portstatus = le16_to_cpu(data.wPortStatus);
2663 return port_is_power_on(hub, portstatus);
2664 }

---
0-DAY kernel build testing backend Open Source Technology Centre
Fengguang Wu <[email protected]> Intel Corporation

Dan Carpenter

2012-09-12 08:50 UTC #1 lore raw

Re: [balbi-usb:merge-result-for-greg 66/99] drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state

Quoted 9 lines · Fengguang Wu

Hi Felipe,
FYI, there are new smatch warnings show up in
tree:   git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git merge-result-for-greg
head:   23953bde3e4d6aa8780dc054f6ad9882ac63f4f4
commit: e918fa161f510136fce45a524e934fe20e62c8b1 [66/99] Merge tag 'gadget-for-v3.7' into merge-result-for-greg
drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state() error: doing dma on the stack ((null))

Smatch prints out a lot of these warnings. I haven't looked at the rules in a while, so maybe there is a bounce buffer somewhere where it detects stack memory and allocates a DMA'able buffer?

It would be better if the function documentation for usb_control_msg() said that the *data pointer had to be kmalloc()ed.

Also I wonder if Documentation/DMA-API-HOWTO.txt is out of data. Are we allowed to DMA to vmalloc()ed memory yet?

regards,
dan carpenter

Felipe Balbi

2012-09-12 12:02 UTC #2 lore raw

Re: [balbi-usb:merge-result-for-greg 66/99] drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state

Hi,

Quoted 10 lines · Dan Carpenter

Quoted 9 lines · Fengguang Wu
Hi Felipe,
FYI, there are new smatch warnings show up in
tree:   git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git merge-result-for-greg
head:   23953bde3e4d6aa8780dc054f6ad9882ac63f4f4
commit: e918fa161f510136fce45a524e934fe20e62c8b1 [66/99] Merge tag 'gadget-for-v3.7' into merge-result-for-greg
drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state() error: doing dma on the stack ((null))

that's not part of what I handle :-(

And merge-result-for-greg was really a temporary branch, btw :-)

Signature · 1 line

balbi

A message that is not in the archive (sent off-list or not yet received): 20120912120211.GC5732-S8G//mZuvNWo5Im9Ml3/[email protected]

Fengguang Wu

author

2012-09-12 12:09 UTC #4 lore raw

Re: [balbi-usb:merge-result-for-greg 66/99] drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state

Quoted 12 lines · Felipe Balbi

Hi,
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Fengguang Wu
Hi Felipe,
FYI, there are new smatch warnings show up in
tree:   git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git merge-result-for-greg
head:   23953bde3e4d6aa8780dc054f6ad9882ac63f4f4
commit: e918fa161f510136fce45a524e934fe20e62c8b1 [66/99] Merge tag 'gadget-for-v3.7' into merge-result-for-greg

Quoted 5 lines

Quoted 1 line
Quoted 1 line
drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state() error: doing dma on the stack ((null))
that's not part of what I handle :-(
And merge-result-for-greg was really a temporary branch, btw :-)

No problem. I'll stop testing this branch :)

Thanks,
Fengguang

Alan Stern

2012-09-12 13:11 UTC #5 lore raw

Re: [balbi-usb:merge-result-for-greg 66/99] drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state

In reply to Fengguang Wu

Quoted 24 lines · Fengguang Wu

Hi Felipe,
FYI, there are new smatch warnings show up in
tree:   git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git merge-result-for-greg
head:   23953bde3e4d6aa8780dc054f6ad9882ac63f4f4
commit: e918fa161f510136fce45a524e934fe20e62c8b1 [66/99] Merge tag 'gadget-for-v3.7' into merge-result-for-greg
drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state() error: doing dma on the stack ((null)) drivers/usb/core/hub.c:4733 store_port_power_control() info: why not propagate 'ret' from set_port_feature() instead of -5? drivers/usb/core/hub.c:4744 store_port_power_control() info: why not propagate 'ret' from clear_port_feature() instead of -5?
vim +2654 drivers/usb/core/hub.c
2644 return ret;
2645 }
2646
2647 static int usb_get_hub_port_power_state(struct usb_device *hdev, int port1)
2648 {
2649 struct usb_hub *hub = hdev_to_hub(hdev);
2650 struct usb_port_status data;
2651 u16 portstatus;
2652 int ret;
2653
Quoted 1 line
2654 ret = get_port_status(hub->hdev, port1, &data);

Quoted 10 lines

2655 if (ret < 4) {
2656 dev_err(hub->intfdev,
2657 "%s failed (err = %d)\n", __func__, ret);
2658 if (ret >= 0)
2659 ret = -EIO;
2660 return ret;
2661 } else
2662 portstatus = le16_to_cpu(data.wPortStatus);
2663 return port_is_power_on(hub, portstatus);
2664 }

This is a real problem. It looks like it really is necessary to call hub_port_status() here, not get_port_status(). Would you like to fix this?

The reason for changing the return codes from clear_port_feature() and set_port_feature() is simple. The values they return are meant for internal use within the kernel, whereas the return code from store_port_power_control() will be passed back to userspace.

Alan Stern

Lan Tianyu

2012-09-12 14:41 UTC #6 lore raw

Re: [balbi-usb:merge-result-for-greg 66/99] drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state

In reply to Fengguang Wu

Quoted 37 lines · Alan Stern

Quoted 24 lines · Fengguang Wu
Hi Felipe,
FYI, there are new smatch warnings show up in
tree:   git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git merge-result-for-greg
head:   23953bde3e4d6aa8780dc054f6ad9882ac63f4f4
commit: e918fa161f510136fce45a524e934fe20e62c8b1 [66/99] Merge tag 'gadget-for-v3.7' into merge-result-for-greg
drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state() error: doing dma on the stack ((null)) drivers/usb/core/hub.c:4733 store_port_power_control() info: why not propagate 'ret' from set_port_feature() instead of -5? drivers/usb/core/hub.c:4744 store_port_power_control() info: why not propagate 'ret' from clear_port_feature() instead of -5?
vim +2654 drivers/usb/core/hub.c
   2644		return ret;
   2645	}
   2646	
   2647	static int usb_get_hub_port_power_state(struct usb_device *hdev, int port1)
   2648	{
   2649		struct usb_hub *hub = hdev_to_hub(hdev);
   2650		struct usb_port_status data;
   2651		u16 portstatus;
   2652		int ret;
   2653	
Quoted 1 line
2654 ret = get_port_status(hub->hdev, port1, &data);
Quoted 10 lines
   2655		if (ret < 4) {
   2656			dev_err(hub->intfdev,
   2657				"%s failed (err = %d)\n", __func__, ret);
   2658			if (ret >= 0)
   2659				ret = -EIO;
   2660			return ret;
   2661		} else
   2662			portstatus = le16_to_cpu(data.wPortStatus);
   2663		return port_is_power_on(hub, portstatus);
   2664	}

Quoted 4 lines

This is a real problem. It looks like it really is necessary to call hub_port_status() here, not get_port_status(). Would you like to fix this?

Yeah. I'd like to fix it.

Quoted 7 lines

The reason for changing the return codes from clear_port_feature() and set_port_feature() is simple. The values they return are meant for internal use within the kernel, whereas the return code from store_port_power_control() will be passed back to userspace.
Alan Stern

Signature · 3 lines

Best Regards
Tianyu Lan
linux kernel enabling team