Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need
On Mon, 2015-11-30 at 17:09 -0800, Greg Kroah-Hartman wrote: > > that would loop through endpoints so that drivers do not have to > > open-code the loop and we indeed need to fix the drivers that > blindly > > grab endpoints at fixed offsets and expect them to be there and have > > correct types. > > Yes, that would work for one single type of endpoint, but lots of > drivers need/have 2 of the same type/direction, so what would this > function do then? Error out? Hm, that might work, and it would > reduce > a bunch of common code, care to make up a patch for that? Hi, in that case let us go the whole way. Give drivers a way to describe what they need that covers all possibilities up to exactly telling the core what it expects and in which order and numbers. Actually that would be better in the interface matching code path. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Two phy-twl4030-usb fixes for unloading the module
Hi, Here are two fixes for rmmod and PM. These can be merged separately after the review from the MUSB related changes. Regards, Tony Tony Lindgren (2): phy: twl4030-usb: Relase usb phy on unload phy: twl4030-usb: Fix unbalanced pm_runtime_enable on module reload drivers/phy/phy-twl4030-usb.c | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] phy: twl4030-usb: Fix unbalanced pm_runtime_enable on module reload
If we reload phy-twl4030-usb, we get a warning about unbalanced pm_runtime_enable. Let's fix the issue and also fix idling of the device on unload before we attempt to shut it down. If we don't properly idle the PHY before shutting it down on removal, the twl4030 ends up consuming about 62mW of extra power compared to running idle with the module loaded. Cc: Bin Liu Cc: Felipe Balbi Cc: Kishon Vijay Abraham I Cc: NeilBrown Signed-off-by: Tony Lindgren --- drivers/phy/phy-twl4030-usb.c | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c index 79870c4..f96065a 100644 --- a/drivers/phy/phy-twl4030-usb.c +++ b/drivers/phy/phy-twl4030-usb.c @@ -715,6 +715,7 @@ static int twl4030_usb_probe(struct platform_device *pdev) pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_set_autosuspend_delay(&pdev->dev, 2000); pm_runtime_enable(&pdev->dev); + pm_runtime_get_sync(&pdev->dev); /* Our job is to use irqs and status from the power module * to keep the transceiver disabled when nothing's connected. @@ -758,6 +759,13 @@ static int twl4030_usb_remove(struct platform_device *pdev) /* set transceiver mode to power on defaults */ twl4030_usb_set_mode(twl, -1); + /* idle ulpi before powering off */ + if (cable_present(twl->linkstat)) + pm_runtime_put_noidle(twl->dev); + pm_runtime_mark_last_busy(twl->dev); + pm_runtime_put_sync_suspend(twl->dev); + pm_runtime_disable(twl->dev); + /* autogate 60MHz ULPI clock, * clear dpll clock request for i2c access, * disable 32KHz @@ -772,11 +780,6 @@ static int twl4030_usb_remove(struct platform_device *pdev) /* disable complete OTG block */ twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB); - if (cable_present(twl->linkstat)) - pm_runtime_put_noidle(twl->dev); - pm_runtime_mark_last_busy(twl->dev); - pm_runtime_put(twl->dev); - return 0; } -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] phy: twl4030-usb: Relase usb phy on unload
Otherwise rmmod omap2430; rmmod phy-twl4030-usb; modprobe omap2430 will try to use a non-existing phy and oops: Unable to handle kernel paging request at virtual address b6f7c1f0 ... [] (devm_usb_get_phy_by_node) from [] (omap2430_musb_init+0x44/0x2b4 [omap2430]) [] (omap2430_musb_init [omap2430]) from [] (musb_init_controller+0x194/0x878 [musb_hdrc]) Cc: Bin Liu Cc: Felipe Balbi Cc: Kishon Vijay Abraham I Cc: NeilBrown Signed-off-by: Tony Lindgren --- drivers/phy/phy-twl4030-usb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c index 3a707dd..79870c4 100644 --- a/drivers/phy/phy-twl4030-usb.c +++ b/drivers/phy/phy-twl4030-usb.c @@ -750,6 +750,7 @@ static int twl4030_usb_remove(struct platform_device *pdev) struct twl4030_usb *twl = platform_get_drvdata(pdev); int val; + usb_remove_phy(&twl->phy); pm_runtime_get_sync(twl->dev); cancel_delayed_work(&twl->id_workaround_work); device_remove_file(twl->dev, &dev_attr_vbus); -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] usb: musb: Fix unbalanced pm_runtime_enable
When reloading omap2430 kernel module we get a warning about
unbalanced pm_runtime_enable. Let's fix this. Note that we
need to do this after the child musb-core platform_device is
removed because of pm_runtime_irq_safe being set at the child.
Cc: Bin Liu
Cc: Felipe Balbi
Cc: Kishon Vijay Abraham I
Cc: NeilBrown
Signed-off-by: Tony Lindgren
---
drivers/usb/musb/omap2430.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index bf05f80..c84e0322 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -664,8 +664,11 @@ static int omap2430_remove(struct platform_device *pdev)
{
struct omap2430_glue*glue = platform_get_drvdata(pdev);
+ pm_runtime_get_sync(glue->dev);
cancel_work_sync(&glue->omap_musb_mailbox_work);
platform_device_unregister(glue->musb);
+ pm_runtime_put_sync(glue->dev);
+ pm_runtime_disable(glue->dev);
return 0;
}
--
2.6.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: musb: core: Fix handling of the phy notifications
We currently can't unload omap2430 MUSB platform glue driver module and
this cause issues for fixing the MUSB code further. The reason we can't
remove omap2430 is because it uses the PHY functions and also exports the
omap_musb_mailbox function that some PHY drivers are using.
Let's fix the issue by exporting a more generic musb_mailbox function
from the MUSB core and allow platform glue layers to register phy_callback
function as needed.
And now we can now also get rid of the include/linux/musb-omap.h.
Cc: Bin Liu
Cc: Felipe Balbi
Cc: Kishon Vijay Abraham I
Cc: NeilBrown
Signed-off-by: Tony Lindgren
---
Probably best that Felipe merges this patch via the USB tree after
comments if that works for Kishon? I have another two fixes for the
phy-twl4030-usb.c coming after this series but they can be merged
separately and won't conflict with this patch.
---
drivers/phy/phy-twl4030-usb.c | 32
drivers/usb/musb/musb_core.c | 21 +
drivers/usb/musb/musb_core.h | 2 ++
drivers/usb/musb/omap2430.c | 27 ++-
drivers/usb/phy/phy-twl6030-usb.c | 30 +++---
include/linux/usb/musb-omap.h | 30 --
include/linux/usb/musb.h | 15 +++
7 files changed, 83 insertions(+), 74 deletions(-)
delete mode 100644 include/linux/usb/musb-omap.h
diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 3a707dd..4a3fc6e 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -34,7 +34,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
@@ -148,10 +148,10 @@
* If VBUS is valid or ID is ground, then we know a
* cable is present and we need to be runtime-enabled
*/
-static inline bool cable_present(enum omap_musb_vbus_id_status stat)
+static inline bool cable_present(enum musb_vbus_id_status stat)
{
- return stat == OMAP_MUSB_VBUS_VALID ||
- stat == OMAP_MUSB_ID_GROUND;
+ return stat == MUSB_VBUS_VALID ||
+ stat == MUSB_ID_GROUND;
}
struct twl4030_usb {
@@ -170,7 +170,7 @@ struct twl4030_usb {
enum twl4030_usb_mode usb_mode;
int irq;
- enum omap_musb_vbus_id_status linkstat;
+ enum musb_vbus_id_status linkstat;
boolvbus_supplied;
struct delayed_work id_workaround_work;
@@ -276,11 +276,11 @@ static bool twl4030_is_driving_vbus(struct twl4030_usb
*twl)
return (ret & (ULPI_OTG_DRVVBUS | ULPI_OTG_CHRGVBUS)) ? true : false;
}
-static enum omap_musb_vbus_id_status
+static enum musb_vbus_id_status
twl4030_usb_linkstat(struct twl4030_usb *twl)
{
int status;
- enum omap_musb_vbus_id_status linkstat = OMAP_MUSB_UNKNOWN;
+ enum musb_vbus_id_status linkstat = MUSB_UNKNOWN;
twl->vbus_supplied = false;
@@ -306,14 +306,14 @@ static enum omap_musb_vbus_id_status
}
if (status & BIT(2))
- linkstat = OMAP_MUSB_ID_GROUND;
+ linkstat = MUSB_ID_GROUND;
else if (status & BIT(7))
- linkstat = OMAP_MUSB_VBUS_VALID;
+ linkstat = MUSB_VBUS_VALID;
else
- linkstat = OMAP_MUSB_VBUS_OFF;
+ linkstat = MUSB_VBUS_OFF;
} else {
- if (twl->linkstat != OMAP_MUSB_UNKNOWN)
- linkstat = OMAP_MUSB_VBUS_OFF;
+ if (twl->linkstat != MUSB_UNKNOWN)
+ linkstat = MUSB_VBUS_OFF;
}
dev_dbg(twl->dev, "HW_CONDITIONS 0x%02x/%d; link %d\n",
@@ -535,7 +535,7 @@ static DEVICE_ATTR(vbus, 0444, twl4030_usb_vbus_show, NULL);
static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
{
struct twl4030_usb *twl = _twl;
- enum omap_musb_vbus_id_status status;
+ enum musb_vbus_id_status status;
bool status_changed = false;
status = twl4030_usb_linkstat(twl);
@@ -567,11 +567,11 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
pm_runtime_mark_last_busy(twl->dev);
pm_runtime_put_autosuspend(twl->dev);
}
- omap_musb_mailbox(status);
+ musb_mailbox(status);
}
/* don't schedule during sleep - irq works right then */
- if (status == OMAP_MUSB_ID_GROUND && pm_runtime_active(twl->dev)) {
+ if (status == MUSB_ID_GROUND && pm_runtime_active(twl->dev)) {
cancel_delayed_work(&twl->id_workaround_work);
schedule_delayed_work(&twl->id_workaround_work, HZ);
}
@@ -670,7 +670,7 @@ static int twl4030_usb_probe(struct platform_device *pdev)
twl->dev= &pdev->dev;
twl->irq= platform_get_irq(pdev, 0);
twl->
[PATCH 0/2] Fix rmmod for musb omap2430 glue layer
Hi, Here are two patches to fix rmmod for musb omap2430. This makes any further work on this driver a bit easier. Regards, Tony Tony Lindgren (2): usb: musb: core: Fix handling of the phy notifications usb: musb: Fix unbalanced pm_runtime_enable drivers/phy/phy-twl4030-usb.c | 32 drivers/usb/musb/musb_core.c | 21 + drivers/usb/musb/musb_core.h | 2 ++ drivers/usb/musb/omap2430.c | 30 +- drivers/usb/phy/phy-twl6030-usb.c | 30 +++--- include/linux/usb/musb-omap.h | 30 -- include/linux/usb/musb.h | 15 +++ 7 files changed, 86 insertions(+), 74 deletions(-) delete mode 100644 include/linux/usb/musb-omap.h -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need
On Mon, Nov 30, 2015 at 04:47:18PM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 30, 2015 at 3:36 PM, Greg Kroah-Hartman
> wrote:
> > On Mon, Nov 30, 2015 at 02:56:09PM -0800, Dmitry Torokhov wrote:
> >> On Mon, Nov 30, 2015 at 2:18 PM, Greg Kroah-Hartman
> >> wrote:
> >> > On Mon, Nov 30, 2015 at 01:11:50PM -0800, Dmitry Torokhov wrote:
> >> >> USB interface drivers need to check number of endpoints before trying to
> >> >> access/use them. Quite a few drivers only use the default setting
> >> >> (altsetting 0), so let's allow them to declare number of endpoints in
> >> >> altsetting 0 they require to operate and have USB core check it for us
> >> >> instead of having every driver implement check manually.
> >> >>
> >> >> For compatibility, if driver does not specify number of endpoints (i.e.
> >> >> number of endpoints is left at 0) we bypass the check in USB core and
> >> >> expect the driver perform necessary checks on its own.
> >> >>
> >> >> Acked-by: Alan Stern
> >> >> Signed-off-by: Dmitry Torokhov
> >> >> ---
> >> >>
> >> >> Greg, if the patch is reasonable I wonder if I can take it through my
> >> >> tree, as I have a few drivers that do not check number of endpoints
> >> >> properly and will crash the kernel when specially crafted device is
> >> >> plugged in, as reported by Vladis Dronov.
> >> >>
> >> >> drivers/usb/core/driver.c | 9 +
> >> >> include/linux/usb.h | 7 +++
> >> >> 2 files changed, 16 insertions(+)
> >> >>
> >> >> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> >> >> index 6b5063e..d9f680d 100644
> >> >> --- a/drivers/usb/core/driver.c
> >> >> +++ b/drivers/usb/core/driver.c
> >> >> @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
> >> >>
> >> >> dev_dbg(dev, "%s - got id\n", __func__);
> >> >>
> >> >> + if (driver->num_endpoints &&
> >> >> + intf->altsetting[0].desc.bNumEndpoints <
> >> >> driver->num_endpoints) {
> >> >> +
> >> >
> >> > Empty line :(
> >> >
> >> >> + dev_err(dev, "Not enough endpoints %d (want %d)\n",
> >> >> + intf->altsetting[0].desc.bNumEndpoints,
> >> >> + driver->num_endpoints);
> >> >
> >> > What can a user do with this?
> >>
> >> Report on the lists or throw such device into a bin.
> >>
> >> >
> >> >> + return -EINVAL;
> >> >> + }
> >> >> +
> >> >> error = usb_autoresume_device(udev);
> >> >> if (error)
> >> >> return error;
> >> >> diff --git a/include/linux/usb.h b/include/linux/usb.h
> >> >> index 447fe29..93f8dfc 100644
> >> >> --- a/include/linux/usb.h
> >> >> +++ b/include/linux/usb.h
> >> >> @@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
> >> >> * @id_table: USB drivers use ID table to support hotplugging.
> >> >> * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set
> >> >> * or your driver's probe function will never get called.
> >> >> + * @num_endpoints: Number of endpoints that should be present in
> >> >> default
> >> >> + * setting (altsetting 0) the driver needs to operate properly.
> >> >> + * The probe will be aborted if actual number of endpoints is less
> >> >> + * than what the driver specified here. 0 means no check should be
> >> >> + * performed.
> >> >
> >> > I don't understand, a driver can do whatever it wants with the endpoints
> >> > of the interface, why do we need to check/know this ahead of time? What
> >> > is crashing without this?
> >>
> >> The kernel because some drivers do not verify that
> >> intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing
> >> intf->altsetting[0].endpoints[0].
> >
> > The USB core does that? Or just a driver, and if it's just a driver, we
> > should fix that in the driver itself as there are lots of other
> > validation checks the drivers should be doing becides just this one
> > about endpoints, sizes, and directions that we can't catch in the core.
> >
> >> > It's up to the driver to check this, if it cares about it.
> >>
> >> Instead of duplicating the check in almost every driver is it more
> >> efficient to allow USB core check it for them (if driver requests it
> >> to do so).
> >
> > ok, fair enough, but it's just one of many things they should be
> > checking, this doesn't provide all that much "protection".
> >
> >> > How many
> >> > drivers do you have that is going to care?
> >>
> >> I saw at least 3 that did not check, that's from cursory glance. Plus
> >> we have many that do check explicitly.
> >>
> >> > Why is this suddenly a new
> >> > thing that we haven't run into in the past 15+ years?
> >>
> >> We are less trusting now. Before we/some of the drivers believed that
> >> if device has VID/PID that they recognize the rest of descriptors will
> >> have the data we expect, but we can't rely on this anymore.
> >
> > There's lots of things we can't "rely on", and we have never been able
> > to rely on, but this is going to require we touch every USB driver to
> > ma
USB resume issue
I'm seeing a problem that looks like an issue in the USB-Persist feature. I'm finding that all my 3.0 thumb drives are torn down and brought back up (fail USB-Persist) during resume from "suspend to memory" if they are plugged into a 2.0 EHCI/OHCI only port on my embedded system. This embedded system seems to be unusual because it removes VBUS during the suspend, but it looks like USB-Persist was written to handle this and normal 2.0 thumb drives work correctly. What I find is that the 3.0 thumb drives take 200ms-500ms after Port Power is applied to set the "Current Connection Status" bit in the PORTSC register and this causes check_port_resume_type() in hub.c to return ENODEV. My 2.0 thumb drives show CONNECTED in < 100ms and this works because hub_power_on() in hub.c waits 100ms for the power to become stable. It looks to me like check_port_resume_type() should look for the CONNECTED bit for hundreds of ms. I checked my Dell laptop to see why it wasn't having a problem with the 3.0 thumb drives and found that pm-suspend leaves the port power on and pm-hibernate re-enables the port power in the BIOS which gives much more than a 500ms delay before the kernel resume code checks for CONNECTED. A recent patch, 7fa40910e0bf5ef32eca49595d950cb24f6402bf, added a CONNECTED retry for a different reason and I could simply increase this retry time. Any thoughts? Thanks Al Cooper -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need
On Mon, Nov 30, 2015 at 3:36 PM, Greg Kroah-Hartman
wrote:
> On Mon, Nov 30, 2015 at 02:56:09PM -0800, Dmitry Torokhov wrote:
>> On Mon, Nov 30, 2015 at 2:18 PM, Greg Kroah-Hartman
>> wrote:
>> > On Mon, Nov 30, 2015 at 01:11:50PM -0800, Dmitry Torokhov wrote:
>> >> USB interface drivers need to check number of endpoints before trying to
>> >> access/use them. Quite a few drivers only use the default setting
>> >> (altsetting 0), so let's allow them to declare number of endpoints in
>> >> altsetting 0 they require to operate and have USB core check it for us
>> >> instead of having every driver implement check manually.
>> >>
>> >> For compatibility, if driver does not specify number of endpoints (i.e.
>> >> number of endpoints is left at 0) we bypass the check in USB core and
>> >> expect the driver perform necessary checks on its own.
>> >>
>> >> Acked-by: Alan Stern
>> >> Signed-off-by: Dmitry Torokhov
>> >> ---
>> >>
>> >> Greg, if the patch is reasonable I wonder if I can take it through my
>> >> tree, as I have a few drivers that do not check number of endpoints
>> >> properly and will crash the kernel when specially crafted device is
>> >> plugged in, as reported by Vladis Dronov.
>> >>
>> >> drivers/usb/core/driver.c | 9 +
>> >> include/linux/usb.h | 7 +++
>> >> 2 files changed, 16 insertions(+)
>> >>
>> >> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>> >> index 6b5063e..d9f680d 100644
>> >> --- a/drivers/usb/core/driver.c
>> >> +++ b/drivers/usb/core/driver.c
>> >> @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
>> >>
>> >> dev_dbg(dev, "%s - got id\n", __func__);
>> >>
>> >> + if (driver->num_endpoints &&
>> >> + intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints)
>> >> {
>> >> +
>> >
>> > Empty line :(
>> >
>> >> + dev_err(dev, "Not enough endpoints %d (want %d)\n",
>> >> + intf->altsetting[0].desc.bNumEndpoints,
>> >> + driver->num_endpoints);
>> >
>> > What can a user do with this?
>>
>> Report on the lists or throw such device into a bin.
>>
>> >
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> error = usb_autoresume_device(udev);
>> >> if (error)
>> >> return error;
>> >> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> >> index 447fe29..93f8dfc 100644
>> >> --- a/include/linux/usb.h
>> >> +++ b/include/linux/usb.h
>> >> @@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
>> >> * @id_table: USB drivers use ID table to support hotplugging.
>> >> * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set
>> >> * or your driver's probe function will never get called.
>> >> + * @num_endpoints: Number of endpoints that should be present in default
>> >> + * setting (altsetting 0) the driver needs to operate properly.
>> >> + * The probe will be aborted if actual number of endpoints is less
>> >> + * than what the driver specified here. 0 means no check should be
>> >> + * performed.
>> >
>> > I don't understand, a driver can do whatever it wants with the endpoints
>> > of the interface, why do we need to check/know this ahead of time? What
>> > is crashing without this?
>>
>> The kernel because some drivers do not verify that
>> intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing
>> intf->altsetting[0].endpoints[0].
>
> The USB core does that? Or just a driver, and if it's just a driver, we
> should fix that in the driver itself as there are lots of other
> validation checks the drivers should be doing becides just this one
> about endpoints, sizes, and directions that we can't catch in the core.
>
>> > It's up to the driver to check this, if it cares about it.
>>
>> Instead of duplicating the check in almost every driver is it more
>> efficient to allow USB core check it for them (if driver requests it
>> to do so).
>
> ok, fair enough, but it's just one of many things they should be
> checking, this doesn't provide all that much "protection".
>
>> > How many
>> > drivers do you have that is going to care?
>>
>> I saw at least 3 that did not check, that's from cursory glance. Plus
>> we have many that do check explicitly.
>>
>> > Why is this suddenly a new
>> > thing that we haven't run into in the past 15+ years?
>>
>> We are less trusting now. Before we/some of the drivers believed that
>> if device has VID/PID that they recognize the rest of descriptors will
>> have the data we expect, but we can't rely on this anymore.
>
> There's lots of things we can't "rely on", and we have never been able
> to rely on, but this is going to require we touch every USB driver to
> make those changes, this one change isn't going to really do all that
> much to help out with that.
>
> Every USB driver _should_ be having a loop over all endpoints to find
> what they need/expect, and if it isn't there, then it needs to abort.
> Just checking the number of endpoints isn't ok, so
Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need
On Mon, Nov 30, 2015 at 02:56:09PM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 30, 2015 at 2:18 PM, Greg Kroah-Hartman
> wrote:
> > On Mon, Nov 30, 2015 at 01:11:50PM -0800, Dmitry Torokhov wrote:
> >> USB interface drivers need to check number of endpoints before trying to
> >> access/use them. Quite a few drivers only use the default setting
> >> (altsetting 0), so let's allow them to declare number of endpoints in
> >> altsetting 0 they require to operate and have USB core check it for us
> >> instead of having every driver implement check manually.
> >>
> >> For compatibility, if driver does not specify number of endpoints (i.e.
> >> number of endpoints is left at 0) we bypass the check in USB core and
> >> expect the driver perform necessary checks on its own.
> >>
> >> Acked-by: Alan Stern
> >> Signed-off-by: Dmitry Torokhov
> >> ---
> >>
> >> Greg, if the patch is reasonable I wonder if I can take it through my
> >> tree, as I have a few drivers that do not check number of endpoints
> >> properly and will crash the kernel when specially crafted device is
> >> plugged in, as reported by Vladis Dronov.
> >>
> >> drivers/usb/core/driver.c | 9 +
> >> include/linux/usb.h | 7 +++
> >> 2 files changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> >> index 6b5063e..d9f680d 100644
> >> --- a/drivers/usb/core/driver.c
> >> +++ b/drivers/usb/core/driver.c
> >> @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
> >>
> >> dev_dbg(dev, "%s - got id\n", __func__);
> >>
> >> + if (driver->num_endpoints &&
> >> + intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
> >> +
> >
> > Empty line :(
> >
> >> + dev_err(dev, "Not enough endpoints %d (want %d)\n",
> >> + intf->altsetting[0].desc.bNumEndpoints,
> >> + driver->num_endpoints);
> >
> > What can a user do with this?
>
> Report on the lists or throw such device into a bin.
>
> >
> >> + return -EINVAL;
> >> + }
> >> +
> >> error = usb_autoresume_device(udev);
> >> if (error)
> >> return error;
> >> diff --git a/include/linux/usb.h b/include/linux/usb.h
> >> index 447fe29..93f8dfc 100644
> >> --- a/include/linux/usb.h
> >> +++ b/include/linux/usb.h
> >> @@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
> >> * @id_table: USB drivers use ID table to support hotplugging.
> >> * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set
> >> * or your driver's probe function will never get called.
> >> + * @num_endpoints: Number of endpoints that should be present in default
> >> + * setting (altsetting 0) the driver needs to operate properly.
> >> + * The probe will be aborted if actual number of endpoints is less
> >> + * than what the driver specified here. 0 means no check should be
> >> + * performed.
> >
> > I don't understand, a driver can do whatever it wants with the endpoints
> > of the interface, why do we need to check/know this ahead of time? What
> > is crashing without this?
>
> The kernel because some drivers do not verify that
> intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing
> intf->altsetting[0].endpoints[0].
The USB core does that? Or just a driver, and if it's just a driver, we
should fix that in the driver itself as there are lots of other
validation checks the drivers should be doing becides just this one
about endpoints, sizes, and directions that we can't catch in the core.
> > It's up to the driver to check this, if it cares about it.
>
> Instead of duplicating the check in almost every driver is it more
> efficient to allow USB core check it for them (if driver requests it
> to do so).
ok, fair enough, but it's just one of many things they should be
checking, this doesn't provide all that much "protection".
> > How many
> > drivers do you have that is going to care?
>
> I saw at least 3 that did not check, that's from cursory glance. Plus
> we have many that do check explicitly.
>
> > Why is this suddenly a new
> > thing that we haven't run into in the past 15+ years?
>
> We are less trusting now. Before we/some of the drivers believed that
> if device has VID/PID that they recognize the rest of descriptors will
> have the data we expect, but we can't rely on this anymore.
There's lots of things we can't "rely on", and we have never been able
to rely on, but this is going to require we touch every USB driver to
make those changes, this one change isn't going to really do all that
much to help out with that.
Every USB driver _should_ be having a loop over all endpoints to find
what they need/expect, and if it isn't there, then it needs to abort.
Just checking the number of endpoints isn't ok, so I really think this
isn't going to help all that much in the end...
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to
Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need
On Mon, Nov 30, 2015 at 2:18 PM, Greg Kroah-Hartman
wrote:
> On Mon, Nov 30, 2015 at 01:11:50PM -0800, Dmitry Torokhov wrote:
>> USB interface drivers need to check number of endpoints before trying to
>> access/use them. Quite a few drivers only use the default setting
>> (altsetting 0), so let's allow them to declare number of endpoints in
>> altsetting 0 they require to operate and have USB core check it for us
>> instead of having every driver implement check manually.
>>
>> For compatibility, if driver does not specify number of endpoints (i.e.
>> number of endpoints is left at 0) we bypass the check in USB core and
>> expect the driver perform necessary checks on its own.
>>
>> Acked-by: Alan Stern
>> Signed-off-by: Dmitry Torokhov
>> ---
>>
>> Greg, if the patch is reasonable I wonder if I can take it through my
>> tree, as I have a few drivers that do not check number of endpoints
>> properly and will crash the kernel when specially crafted device is
>> plugged in, as reported by Vladis Dronov.
>>
>> drivers/usb/core/driver.c | 9 +
>> include/linux/usb.h | 7 +++
>> 2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>> index 6b5063e..d9f680d 100644
>> --- a/drivers/usb/core/driver.c
>> +++ b/drivers/usb/core/driver.c
>> @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
>>
>> dev_dbg(dev, "%s - got id\n", __func__);
>>
>> + if (driver->num_endpoints &&
>> + intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
>> +
>
> Empty line :(
>
>> + dev_err(dev, "Not enough endpoints %d (want %d)\n",
>> + intf->altsetting[0].desc.bNumEndpoints,
>> + driver->num_endpoints);
>
> What can a user do with this?
Report on the lists or throw such device into a bin.
>
>> + return -EINVAL;
>> + }
>> +
>> error = usb_autoresume_device(udev);
>> if (error)
>> return error;
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index 447fe29..93f8dfc 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
>> * @id_table: USB drivers use ID table to support hotplugging.
>> * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set
>> * or your driver's probe function will never get called.
>> + * @num_endpoints: Number of endpoints that should be present in default
>> + * setting (altsetting 0) the driver needs to operate properly.
>> + * The probe will be aborted if actual number of endpoints is less
>> + * than what the driver specified here. 0 means no check should be
>> + * performed.
>
> I don't understand, a driver can do whatever it wants with the endpoints
> of the interface, why do we need to check/know this ahead of time? What
> is crashing without this?
The kernel because some drivers do not verify that
intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing
intf->altsetting[0].endpoints[0].
>
> It's up to the driver to check this, if it cares about it.
Instead of duplicating the check in almost every driver is it more
efficient to allow USB core check it for them (if driver requests it
to do so).
> How many
> drivers do you have that is going to care?
I saw at least 3 that did not check, that's from cursory glance. Plus
we have many that do check explicitly.
> Why is this suddenly a new
> thing that we haven't run into in the past 15+ years?
We are less trusting now. Before we/some of the drivers believed that
if device has VID/PID that they recognize the rest of descriptors will
have the data we expect, but we can't rely on this anymore.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: serial: cp210x: Cleaned up USB access functions.
cp210x_get_config and cp210x_set_config were hard to use. They required
the buffer as an array of 32-bit values even for smaller values, and did
endian conversions on per-32-bit value basis, which is wrong for some
cp210x data structures (although not for any that were actually used.)
This change introduces separate register accessor functions for single
8, 16 and 32-bit values, with endian conversion, as well as "block"
access functions without conversion.
Signed-off-by: Konstantin Shkolnyy
---
drivers/usb/serial/cp210x.c | 314 ++--
1 file changed, 186 insertions(+), 128 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index fd67958..ce80d5f 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -323,113 +323,169 @@ struct cp210x_comm_status {
#define PURGE_ALL 0x000f
/*
- * cp210x_get_config
- * Reads from the CP210x configuration registers
- * 'size' is specified in bytes.
- * 'data' is a pointer to a pre-allocated array of integers large
- * enough to hold 'size' bytes (with 4 bytes to each integer)
+ * Reads a variable-sized block of CP210X_ registers, identified by req.
+ * Returns data into buf in native USB byte order.
*/
-static int cp210x_get_config(struct usb_serial_port *port, u8 request,
- unsigned int *data, int size)
+static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req,
+ void *buf, int bufsize)
{
struct usb_serial *serial = port->serial;
struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
- __le32 *buf;
- int result, i, length;
-
- /* Number of integers required to contain the array */
- length = (((size - 1) | 3) + 1) / 4;
+ void *dmabuf;
+ int result;
- buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
- if (!buf)
+ dmabuf = kmalloc(bufsize, GFP_KERNEL);
+ if (!dmabuf) {
+ /*
+* FIXME Some callers don't bother to check for error,
+* at least give them consistent junk until they are fixed
+*/
+ memset(buf, 0, bufsize);
return -ENOMEM;
+ }
- /* Issue the request, attempting to read 'size' bytes */
result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
- request, REQTYPE_INTERFACE_TO_HOST, 0x,
- port_priv->bInterfaceNumber, buf, size,
- USB_CTRL_GET_TIMEOUT);
+ req, REQTYPE_INTERFACE_TO_HOST, 0,
+ port_priv->bInterfaceNumber, dmabuf, bufsize,
+ USB_CTRL_SET_TIMEOUT);
+ if (result == bufsize) {
+ memcpy(buf, dmabuf, bufsize);
+ result = 0;
+ } else {
+ dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n",
+ req, bufsize, result);
+ if (result >= 0)
+ result = -EPROTO;
- /* Convert data into an array of integers */
- for (i = 0; i < length; i++)
- data[i] = le32_to_cpu(buf[i]);
+ /*
+* FIXME Some callers don't bother to check for error,
+* at least give them consistent junk until they are fixed
+*/
+ memset(buf, 0, bufsize);
+ }
- kfree(buf);
+ kfree(dmabuf);
- if (result != size) {
- dev_dbg(&port->dev, "%s - Unable to send config request,
request=0x%x size=%d result=%d\n",
- __func__, request, size, result);
- if (result > 0)
- result = -EPROTO;
+ return result;
+}
+
+/*
+ * Reads any 32-bit CP210X_ register identified by req.
+ */
+static int cp210x_read_u32_reg(struct usb_serial_port *port, u8 req, u32 *val)
+{
+ __le32 le32_val;
+ int err;
- return result;
+ err = cp210x_read_reg_block(port, req, &le32_val, sizeof(le32_val));
+ if (err) {
+ /*
+* FIXME Some callers don't bother to check for error,
+* at least give them consistent junk until they are fixed
+*/
+ *val = 0;
+ return err;
}
+ *val = le32_to_cpu(le32_val);
+
return 0;
}
/*
- * cp210x_set_config
- * Writes to the CP210x configuration registers
- * Values less than 16 bits wide are sent directly
- * 'size' is specified in bytes.
+ * Reads any 16-bit CP210X_ register identified by req.
*/
-static int cp210x_set_config(struct usb_serial_port *port, u8 request,
- unsigned int *data, int size)
+static int cp210x_read_u16_reg(struct usb_serial_port *port, u8 req, u16 *val)
+{
+ __le16 le16_val;
+ int err;
+
+ err = cp210x_read_reg_block(port, req, &le16_val, sizeo
Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need
On Mon, Nov 30, 2015 at 03:39:43PM -0600, Felipe Balbi wrote:
>
> Hi,
>
> Dmitry Torokhov writes:
> > USB interface drivers need to check number of endpoints before trying to
> > access/use them. Quite a few drivers only use the default setting
> > (altsetting 0), so let's allow them to declare number of endpoints in
> > altsetting 0 they require to operate and have USB core check it for us
> > instead of having every driver implement check manually.
> >
> > For compatibility, if driver does not specify number of endpoints (i.e.
> > number of endpoints is left at 0) we bypass the check in USB core and
> > expect the driver perform necessary checks on its own.
> >
> > Acked-by: Alan Stern
> > Signed-off-by: Dmitry Torokhov
> > ---
> >
> > Greg, if the patch is reasonable I wonder if I can take it through my
> > tree, as I have a few drivers that do not check number of endpoints
> > properly and will crash the kernel when specially crafted device is
> > plugged in, as reported by Vladis Dronov.
> >
> > drivers/usb/core/driver.c | 9 +
> > include/linux/usb.h | 7 +++
> > 2 files changed, 16 insertions(+)
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index 6b5063e..d9f680d 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
> >
> > dev_dbg(dev, "%s - got id\n", __func__);
> >
> > + if (driver->num_endpoints &&
>
> this part of the check is pointless, right ?
>
> > + intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
>
> bNumEndpoints will never be less than 0 and if it is, we're gonna have
> issues elsewhere anyway.
Fair enough, I'll drop it.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need
On Mon, Nov 30, 2015 at 01:11:50PM -0800, Dmitry Torokhov wrote:
> USB interface drivers need to check number of endpoints before trying to
> access/use them. Quite a few drivers only use the default setting
> (altsetting 0), so let's allow them to declare number of endpoints in
> altsetting 0 they require to operate and have USB core check it for us
> instead of having every driver implement check manually.
>
> For compatibility, if driver does not specify number of endpoints (i.e.
> number of endpoints is left at 0) we bypass the check in USB core and
> expect the driver perform necessary checks on its own.
>
> Acked-by: Alan Stern
> Signed-off-by: Dmitry Torokhov
> ---
>
> Greg, if the patch is reasonable I wonder if I can take it through my
> tree, as I have a few drivers that do not check number of endpoints
> properly and will crash the kernel when specially crafted device is
> plugged in, as reported by Vladis Dronov.
>
> drivers/usb/core/driver.c | 9 +
> include/linux/usb.h | 7 +++
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 6b5063e..d9f680d 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
>
> dev_dbg(dev, "%s - got id\n", __func__);
>
> + if (driver->num_endpoints &&
> + intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
> +
Empty line :(
> + dev_err(dev, "Not enough endpoints %d (want %d)\n",
> + intf->altsetting[0].desc.bNumEndpoints,
> + driver->num_endpoints);
What can a user do with this?
> + return -EINVAL;
> + }
> +
> error = usb_autoresume_device(udev);
> if (error)
> return error;
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 447fe29..93f8dfc 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
> * @id_table: USB drivers use ID table to support hotplugging.
> * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set
> * or your driver's probe function will never get called.
> + * @num_endpoints: Number of endpoints that should be present in default
> + * setting (altsetting 0) the driver needs to operate properly.
> + * The probe will be aborted if actual number of endpoints is less
> + * than what the driver specified here. 0 means no check should be
> + * performed.
I don't understand, a driver can do whatever it wants with the endpoints
of the interface, why do we need to check/know this ahead of time? What
is crashing without this?
It's up to the driver to check this, if it cares about it. How many
drivers do you have that is going to care? Why is this suddenly a new
thing that we haven't run into in the past 15+ years?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Infrastructure for zerocopy I/O
On Mon, 30 Nov 2015, Oliver Neukum wrote:
> On Mon, 2015-11-30 at 12:14 -0500, Alan Stern wrote:
> > > +static struct usb_memory *
> > > +find_memory_area(struct usb_dev_state *ps, const struct
> > usbdevfs_urb *uurb)
> > > +{
> > > + struct usb_memory *usbm = NULL, *iter;
> > > + unsigned long flags;
> > > + unsigned long uurb_start = (unsigned long)uurb->buffer;
> > > +
> > > + spin_lock_irqsave(&ps->lock, flags);
> > > + list_for_each_entry(iter, &ps->memory_list, memlist) {
> > > + if (iter->usb_use_count == 0 &&
> >
> > I don't think this is necessary. It should be legal to keep the data
> > for two URBs in the same memory region, so long as they don't overlap.
>
> Hi,
>
> they also must not share cache lines. How do you guarantee that in this
> case?
I can't guarantee it -- that's the responsibility of the user program.
If it wants to put two transfer buffers in the same memory region, it
should know about the potential problems.
Besides, if we use dma_zalloc_coherent() to allocate the memory then
the problem doesn't arise. Coherent memory can safely be accessed by
both the device and the CPU at any time, by definition, without regard
for caching.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Infrastructure for zerocopy I/O
On Mon, 2015-11-30 at 12:14 -0500, Alan Stern wrote:
> > +static struct usb_memory *
> > +find_memory_area(struct usb_dev_state *ps, const struct
> usbdevfs_urb *uurb)
> > +{
> > + struct usb_memory *usbm = NULL, *iter;
> > + unsigned long flags;
> > + unsigned long uurb_start = (unsigned long)uurb->buffer;
> > +
> > + spin_lock_irqsave(&ps->lock, flags);
> > + list_for_each_entry(iter, &ps->memory_list, memlist) {
> > + if (iter->usb_use_count == 0 &&
>
> I don't think this is necessary. It should be legal to keep the data
> for two URBs in the same memory region, so long as they don't overlap.
Hi,
they also must not share cache lines. How do you guarantee that in this
case?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need
Hi,
Dmitry Torokhov writes:
> USB interface drivers need to check number of endpoints before trying to
> access/use them. Quite a few drivers only use the default setting
> (altsetting 0), so let's allow them to declare number of endpoints in
> altsetting 0 they require to operate and have USB core check it for us
> instead of having every driver implement check manually.
>
> For compatibility, if driver does not specify number of endpoints (i.e.
> number of endpoints is left at 0) we bypass the check in USB core and
> expect the driver perform necessary checks on its own.
>
> Acked-by: Alan Stern
> Signed-off-by: Dmitry Torokhov
> ---
>
> Greg, if the patch is reasonable I wonder if I can take it through my
> tree, as I have a few drivers that do not check number of endpoints
> properly and will crash the kernel when specially crafted device is
> plugged in, as reported by Vladis Dronov.
>
> drivers/usb/core/driver.c | 9 +
> include/linux/usb.h | 7 +++
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 6b5063e..d9f680d 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
>
> dev_dbg(dev, "%s - got id\n", __func__);
>
> + if (driver->num_endpoints &&
this part of the check is pointless, right ?
> + intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
bNumEndpoints will never be less than 0 and if it is, we're gonna have
issues elsewhere anyway.
--
balbi
signature.asc
Description: PGP signature
[PATCH] usb: dwc3: gadget: don't prestart interrupt endpoints
Because interrupt endpoints usually transmit such
small amounts of data, it seems pointless to prestart
transfers and try to get speed improvements. This
patch also sorts out a problem with CDC ECM function
where its notification endpoint gets stuck in busy
state and we continuously issue Update Transfer
commands.
Fixes: 8a1a9c9e4503 ("usb: dwc3: gadget: start transfer on XFER_COMPLETE")
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 55ba447fdf8b..e5bb60242ffa 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1078,6 +1078,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep,
struct dwc3_request *req)
* little bit faster.
*/
if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
+ !usb_endpoint_xfer_int(dep->endpoint.desc) &&
!(dep->flags & DWC3_EP_BUSY)) {
ret = __dwc3_gadget_kick_transfer(dep, 0, true);
goto out;
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: Quiet down false peer failure messages
On Mon, Nov 30, 2015 at 12:57:17PM -0800, Dan Williams wrote: > On Tue, Nov 17, 2015 at 1:00 PM, Don Zickus wrote: > > My recent Intel box is spewing these messages: > > > > xhci_hcd :00:14.0: xHCI Host Controller > > xhci_hcd :00:14.0: new USB bus registered, assigned bus number 2 > > usb usb2: New USB device found, idVendor=1d6b, idProduct=0003 > > usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > > usb usb2: Product: xHCI Host Controller > > usb usb2: Manufacturer: Linux 4.3.0+ xhci-hcd > > usb usb2: SerialNumber: :00:14.0 > > hub 2-0:1.0: USB hub found > > hub 2-0:1.0: 6 ports detected > > usb: failed to peer usb2-port2 and usb1-port1 by location (usb2-port2:none) > > (usb1-port1:usb2-port1) > > usb usb2-port2: failed to peer to usb1-port1 (-16) > > usb: port power management may be unreliable > > usb: failed to peer usb2-port3 and usb1-port1 by location (usb2-port3:none) > > (usb1-port1:usb2-port1) > > usb usb2-port3: failed to peer to usb1-port1 (-16) > > usb: failed to peer usb2-port5 and usb1-port1 by location (usb2-port5:none) > > (usb1-port1:usb2-port1) > > usb usb2-port5: failed to peer to usb1-port1 (-16) > > usb: failed to peer usb2-port6 and usb1-port1 by location (usb2-port6:none) > > (usb1-port1:usb2-port1) > > usb usb2-port6: failed to peer to usb1-port1 (-16) > > > > Diving into the acpi tables, I noticed the EHCI hub has 12 ports while the > > XHCI > > hub has 8 ports. Most of those ports are of connect type USB_PORT_NOT_USED > > (including port 1 of the EHCI hub). > > > > Further the unused ports have location data initialized to 0x8000. > > > > Now each unused port on the xhci hub walks the port list and finds a > > matching > > peer with port1 of the EHCI hub because the zero'd out group id bits > > falsely match. > > After port1 of the XHCI hub, each following matching peer will generate the > > above warning. > > > > These warnings seem to be harmless for this scenario as I don't think it > > matters that unused ports could not create a peer link. > > > > The attached patch utilizes that assumption and just skips the > > find_and_link_peer setup if a port is determined to be unused. This further > > assumes that port's status will never change. > > > > Tested on my Intel box. > > > > Signed-off-by: Don Zickus > > --- > > drivers/usb/core/port.c | 7 +++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > > index 2106183..3a8f84a 100644 > > --- a/drivers/usb/core/port.c > > +++ b/drivers/usb/core/port.c > > @@ -353,6 +353,13 @@ static void find_and_link_peer(struct usb_hub *hub, > > int port1) > > struct usb_hub *peer_hub; > > > > /* > > +* Un-used ports have zero'd out data that can create a false > > +* peer in-use failure. > > +*/ > > + if (port_dev->connect_type == USB_PORT_NOT_USED) > > + return; > > + > > + /* > > * If location data is available then we can only peer this port > > * by a location match, not the default peer (lest we create a > > * situation where we need to go back and undo a default peering > > Looks reasonable, but it may hide real errors in the ACPI tables. > > How about just changing the dev_warn() in link_peers_report() to > dev_dbg? We'll still get the pr_warn_once() to get the notification > that something went wrong, but won't continue to spam the logs if the > user does not care about peer port power management. Ok. Sounds fair. Curious, I couldn't figure out from the specs if this was valid or not, or if my Intel pre-production machine has an early firmware on it that might resolve this issue later.. Cheers, Don -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: Quiet down false peer failure messages
On Mon, Nov 30, 2015 at 03:49:51PM -0500, Alan Stern wrote: > On Mon, 30 Nov 2015, Don Zickus wrote: > > > On Tue, Nov 17, 2015 at 04:00:58PM -0500, Don Zickus wrote: > > > My recent Intel box is spewing these messages: > > > > poke? > > > > Cheers, > > Don > > It's up to Greg KH to accept this patch; maybe you need to mail it to > him. He seems to be busy these days -- at least, he hasn't responded > yet to some patches I sent shortly after the last merge window ended. Ok. > > I'm okay with the basic idea, but it would be good to get Dan Williams > to vet it. He has spent more time tussling with USB-3/2 peering issues > than I have. He responded with some feedback. I will spin a v2 based on it. Thanks! Cheers, Don -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: interface: allow drivers declare number of endpoints they need
USB interface drivers need to check number of endpoints before trying to
access/use them. Quite a few drivers only use the default setting
(altsetting 0), so let's allow them to declare number of endpoints in
altsetting 0 they require to operate and have USB core check it for us
instead of having every driver implement check manually.
For compatibility, if driver does not specify number of endpoints (i.e.
number of endpoints is left at 0) we bypass the check in USB core and
expect the driver perform necessary checks on its own.
Acked-by: Alan Stern
Signed-off-by: Dmitry Torokhov
---
Greg, if the patch is reasonable I wonder if I can take it through my
tree, as I have a few drivers that do not check number of endpoints
properly and will crash the kernel when specially crafted device is
plugged in, as reported by Vladis Dronov.
drivers/usb/core/driver.c | 9 +
include/linux/usb.h | 7 +++
2 files changed, 16 insertions(+)
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 6b5063e..d9f680d 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
dev_dbg(dev, "%s - got id\n", __func__);
+ if (driver->num_endpoints &&
+ intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
+
+ dev_err(dev, "Not enough endpoints %d (want %d)\n",
+ intf->altsetting[0].desc.bNumEndpoints,
+ driver->num_endpoints);
+ return -EINVAL;
+ }
+
error = usb_autoresume_device(udev);
if (error)
return error;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 447fe29..93f8dfc 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
* @id_table: USB drivers use ID table to support hotplugging.
* Export this with MODULE_DEVICE_TABLE(usb,...). This must be set
* or your driver's probe function will never get called.
+ * @num_endpoints: Number of endpoints that should be present in default
+ * setting (altsetting 0) the driver needs to operate properly.
+ * The probe will be aborted if actual number of endpoints is less
+ * than what the driver specified here. 0 means no check should be
+ * performed.
* @dynids: used internally to hold the list of dynamically added device
* ids for this driver.
* @drvwrap: Driver-model core structure wrapper.
@@ -1099,6 +1104,8 @@ struct usb_driver {
const struct usb_device_id *id_table;
+ unsigned int num_endpoints;
+
struct usb_dynids dynids;
struct usbdrv_wrap drvwrap;
unsigned int no_dynamic_id:1;
--
2.6.0.rc2.230.g3dd15c0
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: Quiet down false peer failure messages
On Tue, Nov 17, 2015 at 1:00 PM, Don Zickus wrote: > My recent Intel box is spewing these messages: > > xhci_hcd :00:14.0: xHCI Host Controller > xhci_hcd :00:14.0: new USB bus registered, assigned bus number 2 > usb usb2: New USB device found, idVendor=1d6b, idProduct=0003 > usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > usb usb2: Product: xHCI Host Controller > usb usb2: Manufacturer: Linux 4.3.0+ xhci-hcd > usb usb2: SerialNumber: :00:14.0 > hub 2-0:1.0: USB hub found > hub 2-0:1.0: 6 ports detected > usb: failed to peer usb2-port2 and usb1-port1 by location (usb2-port2:none) > (usb1-port1:usb2-port1) > usb usb2-port2: failed to peer to usb1-port1 (-16) > usb: port power management may be unreliable > usb: failed to peer usb2-port3 and usb1-port1 by location (usb2-port3:none) > (usb1-port1:usb2-port1) > usb usb2-port3: failed to peer to usb1-port1 (-16) > usb: failed to peer usb2-port5 and usb1-port1 by location (usb2-port5:none) > (usb1-port1:usb2-port1) > usb usb2-port5: failed to peer to usb1-port1 (-16) > usb: failed to peer usb2-port6 and usb1-port1 by location (usb2-port6:none) > (usb1-port1:usb2-port1) > usb usb2-port6: failed to peer to usb1-port1 (-16) > > Diving into the acpi tables, I noticed the EHCI hub has 12 ports while the > XHCI > hub has 8 ports. Most of those ports are of connect type USB_PORT_NOT_USED > (including port 1 of the EHCI hub). > > Further the unused ports have location data initialized to 0x8000. > > Now each unused port on the xhci hub walks the port list and finds a matching > peer with port1 of the EHCI hub because the zero'd out group id bits falsely > match. > After port1 of the XHCI hub, each following matching peer will generate the > above warning. > > These warnings seem to be harmless for this scenario as I don't think it > matters that unused ports could not create a peer link. > > The attached patch utilizes that assumption and just skips the > find_and_link_peer setup if a port is determined to be unused. This further > assumes that port's status will never change. > > Tested on my Intel box. > > Signed-off-by: Don Zickus > --- > drivers/usb/core/port.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > index 2106183..3a8f84a 100644 > --- a/drivers/usb/core/port.c > +++ b/drivers/usb/core/port.c > @@ -353,6 +353,13 @@ static void find_and_link_peer(struct usb_hub *hub, int > port1) > struct usb_hub *peer_hub; > > /* > +* Un-used ports have zero'd out data that can create a false > +* peer in-use failure. > +*/ > + if (port_dev->connect_type == USB_PORT_NOT_USED) > + return; > + > + /* > * If location data is available then we can only peer this port > * by a location match, not the default peer (lest we create a > * situation where we need to go back and undo a default peering Looks reasonable, but it may hide real errors in the ACPI tables. How about just changing the dev_warn() in link_peers_report() to dev_dbg? We'll still get the pr_warn_once() to get the notification that something went wrong, but won't continue to spam the logs if the user does not care about peer port power management. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: Quiet down false peer failure messages
On Mon, 30 Nov 2015, Don Zickus wrote: > On Tue, Nov 17, 2015 at 04:00:58PM -0500, Don Zickus wrote: > > My recent Intel box is spewing these messages: > > poke? > > Cheers, > Don It's up to Greg KH to accept this patch; maybe you need to mail it to him. He seems to be busy these days -- at least, he hasn't responded yet to some patches I sent shortly after the last merge window ended. I'm okay with the basic idea, but it would be good to get Dan Williams to vet it. He has spent more time tussling with USB-3/2 peering issues than I have. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: musb: convert printk to pr_*
On Sat, Nov 28 2015, Sergei Shtylyov wrote: > On 11/28/2015 3:04 AM, Greg Kroah-Hartman wrote: > This file already uses pr_debug in a few places; this converts the remaining printks. >>> >>> Are you aware that printk(KERN_DEBUG, ...) and pr_debug() are not >>> equivalent? >> >> Yes, and that is a good thing, you should be using pr_debug() instead of >> printk(KERN_DEBUG...). >> >> Why object to something like this? > >I'm not objecting, just asking. There have been many cases in my > practice where a patch author wasn't aware of that... I was aware, but I can see how the commit message could indicate otherwise. I don't feel strongly for the conversion, so I could redo 2/3 and 3/3 (which should be uncontroversial cleanups). Rasmus -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: Quiet down false peer failure messages
On Tue, Nov 17, 2015 at 04:00:58PM -0500, Don Zickus wrote: > My recent Intel box is spewing these messages: poke? Cheers, Don > > xhci_hcd :00:14.0: xHCI Host Controller > xhci_hcd :00:14.0: new USB bus registered, assigned bus number 2 > usb usb2: New USB device found, idVendor=1d6b, idProduct=0003 > usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > usb usb2: Product: xHCI Host Controller > usb usb2: Manufacturer: Linux 4.3.0+ xhci-hcd > usb usb2: SerialNumber: :00:14.0 > hub 2-0:1.0: USB hub found > hub 2-0:1.0: 6 ports detected > usb: failed to peer usb2-port2 and usb1-port1 by location (usb2-port2:none) > (usb1-port1:usb2-port1) > usb usb2-port2: failed to peer to usb1-port1 (-16) > usb: port power management may be unreliable > usb: failed to peer usb2-port3 and usb1-port1 by location (usb2-port3:none) > (usb1-port1:usb2-port1) > usb usb2-port3: failed to peer to usb1-port1 (-16) > usb: failed to peer usb2-port5 and usb1-port1 by location (usb2-port5:none) > (usb1-port1:usb2-port1) > usb usb2-port5: failed to peer to usb1-port1 (-16) > usb: failed to peer usb2-port6 and usb1-port1 by location (usb2-port6:none) > (usb1-port1:usb2-port1) > usb usb2-port6: failed to peer to usb1-port1 (-16) > > Diving into the acpi tables, I noticed the EHCI hub has 12 ports while the > XHCI > hub has 8 ports. Most of those ports are of connect type USB_PORT_NOT_USED > (including port 1 of the EHCI hub). > > Further the unused ports have location data initialized to 0x8000. > > Now each unused port on the xhci hub walks the port list and finds a matching > peer with port1 of the EHCI hub because the zero'd out group id bits falsely > match. > After port1 of the XHCI hub, each following matching peer will generate the > above warning. > > These warnings seem to be harmless for this scenario as I don't think it > matters that unused ports could not create a peer link. > > The attached patch utilizes that assumption and just skips the > find_and_link_peer setup if a port is determined to be unused. This further > assumes that port's status will never change. > > Tested on my Intel box. > > Signed-off-by: Don Zickus > --- > drivers/usb/core/port.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > index 2106183..3a8f84a 100644 > --- a/drivers/usb/core/port.c > +++ b/drivers/usb/core/port.c > @@ -353,6 +353,13 @@ static void find_and_link_peer(struct usb_hub *hub, int > port1) > struct usb_hub *peer_hub; > > /* > + * Un-used ports have zero'd out data that can create a false > + * peer in-use failure. > + */ > + if (port_dev->connect_type == USB_PORT_NOT_USED) > + return; > + > + /* >* If location data is available then we can only peer this port >* by a location match, not the default peer (lest we create a >* situation where we need to go back and undo a default peering > -- > 2.6.0.rc0.2.g7662973 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
On 11/30/2015 06:20 PM, Greg KH wrote: On Mon, Nov 30, 2015 at 06:12:22PM +0100, Krzysztof Opasiak wrote: On 11/30/2015 05:16 PM, Alan Stern wrote: On Fri, 27 Nov 2015, Krzysztof Opasiak wrote: I run through your code and as far as I understand above is not exactly true. Your patch allows only to prevent userspace from accessing interfaces which has kernel drivers, there is no way to stop an application from taking control over all free interfaces. Let's say that your device has 3 interfaces. First of them has a kernel driver but second and third doesn't. You have 2 apps. One should communicate using second interface and another one third. But first app is malicious and it claims all free interfaces of received device (your patch doesn't prevent this). And when second app starts it is unable to do anything with the device because all interfaces are taken. How would you like to handle this? You can't, and why would you ever want to, as you can't tell what an app "should" or "should not" do. If you really care about this, then use a LSM policy to prevent this. Well, an app can declare what it does and what it needs in it's manifest file (or some equivalent of this) and the platform should ensure that app can do only what it has declared. I would really like to use LSM policy in here but currently it is impossible as one device node represents whole device. Permissions (even those from LSM) are being checked only on open() not on each ioctl() so as far as I know there is nothing which prevents any owner of opened fd to claim all available (not taken by someone else) interfaces and LSM policy is unable to filter those calls (unless we add some LSM hooks over there). How about this approach? Once a process has dropped its usbfs privileges, it's not allowed to claim any interfaces (either explicitly or implicitly). Instead, it or some manager program must claim the appropriate interfaces before dropping privileges. I agree that restricting interface claiming only to privileged process is a good idea. Unfortunately this generates a problem when program needs more than one interface (like in cdc - data + control for example). We need to declare both of them in first call to "usb-manager" or reopen the dev node at second call and claim all interfaces claimed using this fd till now and claim one more and then drop privileges and send a new fd. Have you seen such a device that is controlled this way in userspace? Don't over-engineer something that is probably pretty rare... Yes I have seen such devices (not cdc of course) and they were driven using libusb (vendor specific service + "driver" to bypass publishing protocol code due to kernel's GPL). I have even seen an android app written in java which claims and uses multiple interfaces using android's USB API, so it's real;) -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
On Mon, Nov 30, 2015 at 06:12:22PM +0100, Krzysztof Opasiak wrote: > > > On 11/30/2015 05:16 PM, Alan Stern wrote: > >On Fri, 27 Nov 2015, Krzysztof Opasiak wrote: > > > I run through your code and as far as I understand above is not exactly > true. Your patch allows only to prevent userspace from accessing > interfaces > which has kernel drivers, there is no way to stop an application from > taking > control over all free interfaces. > > Let's say that your device has 3 interfaces. First of them has a kernel > driver but second and third doesn't. You have 2 apps. One should > communicate > using second interface and another one third. But first app is malicious > and > it claims all free interfaces of received device (your patch doesn't > prevent > this). And when second app starts it is unable to do anything with the > device because all interfaces are taken. How would you like to handle > this? > >>> > >>>You can't, and why would you ever want to, as you can't tell what an app > >>>"should" or "should not" do. If you really care about this, then use a > >>>LSM policy to prevent this. > >> > >>Well, an app can declare what it does and what it needs in it's manifest > >>file (or some equivalent of this) and the platform should ensure that > >>app can do only what it has declared. > >> > >>I would really like to use LSM policy in here but currently it is > >>impossible as one device node represents whole device. Permissions (even > >>those from LSM) are being checked only on open() not on each ioctl() so > >>as far as I know there is nothing which prevents any owner of opened fd > >>to claim all available (not taken by someone else) interfaces and LSM > >>policy is unable to filter those calls (unless we add some LSM hooks > >>over there). > > > >How about this approach? Once a process has dropped its usbfs > >privileges, it's not allowed to claim any interfaces (either explicitly > >or implicitly). Instead, it or some manager program must claim the > >appropriate interfaces before dropping privileges. > > > > I agree that restricting interface claiming only to privileged process is a > good idea. Unfortunately this generates a problem when program needs more > than one interface (like in cdc - data + control for example). We need to > declare both of them in first call to "usb-manager" or reopen the dev node > at second call and claim all interfaces claimed using this fd till now and > claim one more and then drop privileges and send a new fd. Have you seen such a device that is controlled this way in userspace? Don't over-engineer something that is probably pretty rare... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
On 11/30/2015 05:16 PM, Alan Stern wrote: On Fri, 27 Nov 2015, Krzysztof Opasiak wrote: I run through your code and as far as I understand above is not exactly true. Your patch allows only to prevent userspace from accessing interfaces which has kernel drivers, there is no way to stop an application from taking control over all free interfaces. Let's say that your device has 3 interfaces. First of them has a kernel driver but second and third doesn't. You have 2 apps. One should communicate using second interface and another one third. But first app is malicious and it claims all free interfaces of received device (your patch doesn't prevent this). And when second app starts it is unable to do anything with the device because all interfaces are taken. How would you like to handle this? You can't, and why would you ever want to, as you can't tell what an app "should" or "should not" do. If you really care about this, then use a LSM policy to prevent this. Well, an app can declare what it does and what it needs in it's manifest file (or some equivalent of this) and the platform should ensure that app can do only what it has declared. I would really like to use LSM policy in here but currently it is impossible as one device node represents whole device. Permissions (even those from LSM) are being checked only on open() not on each ioctl() so as far as I know there is nothing which prevents any owner of opened fd to claim all available (not taken by someone else) interfaces and LSM policy is unable to filter those calls (unless we add some LSM hooks over there). How about this approach? Once a process has dropped its usbfs privileges, it's not allowed to claim any interfaces (either explicitly or implicitly). Instead, it or some manager program must claim the appropriate interfaces before dropping privileges. I agree that restricting interface claiming only to privileged process is a good idea. Unfortunately this generates a problem when program needs more than one interface (like in cdc - data + control for example). We need to declare both of them in first call to "usb-manager" or reopen the dev node at second call and claim all interfaces claimed using this fd till now and claim one more and then drop privileges and send a new fd. Maybe better option would be to add optional argument to claim interface ioctl() and allow to claim interface for other fd than the current one? So "usb-manager" could have fd with full control and claim interfaces for apps which have fds with restricted privileges. Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Infrastructure for zerocopy I/O
On Thu, 26 Nov 2015, Steinar H. Gunderson wrote:
> On Wed, Nov 25, 2015 at 10:29:53AM -0500, Alan Stern wrote:
> > I want to see a modified version of your patch. Several things need to
> > be changed or fixed, but the major change needs to be the way memory is
> > allocated. It should be done as part of the mmap system call, not as a
> > separate ioctl.
>
> I took a stab at this. The attached patch doesn't fix any of your other
> issues, but could you take a look at whether the mmap part looks sane?
> I've verified that it works in practice with my own code, and see no
> performance regressions. It is still the case that you can't mix mmap
> and non-mmap transfers on the same device; I suppose that should be governed
> by a zerocopy flag instead of “are there any mmap-ed areas”.
As I recall, Markus's original patch took care of this by checking to
see whether the transfer buffer was in one of the mmap'ed areas. If it
was then the transfer would be zerocopy; otherwise it would be normal.
That seems like a reasonable approach.
> Let me go through your other issues:
>
> > There are numerous smaller issues: The allocated memory needs to be
> > charged against usbfs_memory_usage
>
> I'll fix this.
>
> > the memory should be allocated using dma_zalloc_coherent instead of
> > kmalloc,
>
> dma_zalloc_coherent returns a dma_handle in addition to the memory itself;
> should we just throw this away?
No, the handle has to be stored for use when an URB is submitted and
when the memory is deallocated -- it is a required argument for
dma_free_coherent().
> > proc_do_submiturb should
> > check that the buffer starts anywhere within the allocated memory
> > rather than just at the beginning
>
> I'll fix this, too.
>
> > , and so on.
>
> Clarification appreciated ;-)
Detailed notes below.
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 38ae877c..9a1a7d6 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -69,6 +69,7 @@ struct usb_dev_state {
> spinlock_t lock;/* protects the async urb lists */
> struct list_head async_pending;
> struct list_head async_completed;
> + struct list_head memory_list;
> wait_queue_head_t wait; /* wake up if a request completed */
> unsigned int discsignr;
> struct pid *disc_pid;
> @@ -96,6 +97,17 @@ struct async {
> u8 bulk_status;
> };
>
> +struct usb_memory {
> + struct list_head memlist;
> + int vma_use_count;
> + int usb_use_count;
> + u32 offset;
What's the reason for the "offset" member? It doesn't seem to do
anything.
> + u32 size;
> + void *mem;
You'll need to store the DMA address as well.
> + unsigned long vm_start;
> + struct usb_dev_state *ps;
> +};
> +
> static bool usbfs_snoop;
> module_param(usbfs_snoop, bool, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(usbfs_snoop, "true to log all usbfs traffic");
> @@ -157,6 +169,74 @@ static int connected(struct usb_dev_state *ps)
> ps->dev->state != USB_STATE_NOTATTACHED);
> }
>
> +static struct usb_memory *
> +alloc_usb_memory(struct usb_dev_state *ps, size_t size)
> +{
> + struct usb_memory *usbm;
> + void *mem;
> + unsigned long flags;
> +
> + mem = alloc_pages_exact(size, GFP_KERNEL | GFP_DMA32);
dma_zalloc_coherent(), not alloc_pages_exact().
> + if (!mem)
> + return NULL;
> +
> + usbm = kzalloc(sizeof(struct usb_memory), GFP_KERNEL);
> + if (!usbm) {
> + free_pages_exact(mem, size);
> + return NULL;
> + }
> + memset(mem, 0x0, PAGE_SIZE << get_order(size));
Then this line won't be needed.
> + usbm->mem = mem;
> + usbm->offset = virt_to_phys(mem);
> + usbm->size = size;
> + usbm->ps = ps;
> + spin_lock_irqsave(&ps->lock, flags);
> + list_add_tail(&usbm->memlist, &ps->memory_list);
> + spin_unlock_irqrestore(&ps->lock, flags);
> +
> + return usbm;
> +}
This subroutine should be merged into usbdev_mmap().
In fact, all the memory-oriented routines should be located together
in the source file. It's confusing to put some of them near the start
and others near the end.
> +
> +static void dec_use_count(struct usb_memory *usbm, int *count)
> +{
> + struct usb_dev_state *ps = usbm->ps;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ps->lock, flags);
> + WARN_ON(count != &usbm->usb_use_count && count != &usbm->vma_use_count);
> + --*count;
> + if (usbm->usb_use_count == 0 && usbm->vma_use_count == 0) {
Forget about the WARN_ON; you know all the callers because this patch
introduces them. If you prefer, instead of a pointer pass an
enumeration value: USB_MEMORY_URB_COUNT or USB_MEMORY_VMA_COUNT.
Also, you might change the name to make it a little less generic. For
example, dec_usb_memory_use_count().
> + list_del_init(&usbm->memlist);
> + free_pages_exact(usbm->mem, usbm->si
Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
On Fri, 27 Nov 2015, Krzysztof Opasiak wrote: > >> I run through your code and as far as I understand above is not exactly > >> true. Your patch allows only to prevent userspace from accessing interfaces > >> which has kernel drivers, there is no way to stop an application from > >> taking > >> control over all free interfaces. > >> > >> Let's say that your device has 3 interfaces. First of them has a kernel > >> driver but second and third doesn't. You have 2 apps. One should > >> communicate > >> using second interface and another one third. But first app is malicious > >> and > >> it claims all free interfaces of received device (your patch doesn't > >> prevent > >> this). And when second app starts it is unable to do anything with the > >> device because all interfaces are taken. How would you like to handle this? > > > > You can't, and why would you ever want to, as you can't tell what an app > > "should" or "should not" do. If you really care about this, then use a > > LSM policy to prevent this. > > Well, an app can declare what it does and what it needs in it's manifest > file (or some equivalent of this) and the platform should ensure that > app can do only what it has declared. > > I would really like to use LSM policy in here but currently it is > impossible as one device node represents whole device. Permissions (even > those from LSM) are being checked only on open() not on each ioctl() so > as far as I know there is nothing which prevents any owner of opened fd > to claim all available (not taken by someone else) interfaces and LSM > policy is unable to filter those calls (unless we add some LSM hooks > over there). How about this approach? Once a process has dropped its usbfs privileges, it's not allowed to claim any interfaces (either explicitly or implicitly). Instead, it or some manager program must claim the appropriate interfaces before dropping privileges. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [TESTPATCH] xhci: fix usb2 resume timing and races.
Hi Mathias,
[auto build test WARNING on: v4.4-rc1]
[cannot apply to: v4.4-rc3 v4.4-rc2 next-20151127]
url:
https://github.com/0day-ci/linux/commits/Mathias-Nyman/xhci-fix-usb2-resume-timing-and-races/20151130-231438
config: i386-randconfig-r0-201548 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers/usb/host/xhci-hub.c: In function 'xhci_get_port_status':
>> drivers/usb/host/xhci-hub.c:812:52: warning: suggest parentheses around '&&'
>> within '||' [-Wparentheses]
(raw_port_status & PORT_PLS_MASK) != XDEV_U3 &&
^
vim +812 drivers/usb/host/xhci-hub.c
796 bus_state->suspended_ports &= ~(1 << wIndex);
797 } else {
798 /*
799 * The resume has been signaling for less than
800 * 20ms. Report the port status as SUSPEND,
801 * let the usbcore check port status again
802 * and clear resume signaling later.
803 */
804 status |= USB_PORT_STAT_SUSPEND;
805 }
806 }
807 /* Clear stale usb2 resume signalling variables in case port
changed
808 * state during 20ms resume signalling. For example on error
809 */
810 if ((bus_state->resume_done[wIndex] ||
811 test_bit(wIndex, &bus_state->resuming_ports) &&
> 812 (raw_port_status & PORT_PLS_MASK) != XDEV_U3 &&
813 (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME)) {
814 bus_state->resume_done[wIndex] = 0;
815 clear_bit(wIndex, &bus_state->resuming_ports);
816 }
817 if ((raw_port_status & PORT_PLS_MASK) == XDEV_U0
818 && (raw_port_status & PORT_POWER)
819 && (bus_state->suspended_ports & (1 <<
wIndex))) {
820 bus_state->suspended_ports &= ~(1 << wIndex);
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: Binary data
[TESTPATCH] xhci: fix usb2 resume timing and races.
usb2 ports need to signal resume for 20ms before moving to U0 state.
Both device and host can initiate resume.
On host initated resume port is set to resume state, sleep 20ms,
and finally set port to U0 state.
On device initated resume a port status interrupt with a port in resume
state in issued. The interrupt handler tags a resume_done[port]
timestamp with current time + 20ms, and kick roothub timer.
Root hub timer requests for port status, finds the port in resume state,
checks if resume_done[port] timestamp passed, and set port to U0 state.
There are a few issues with this approach,
1. A host initated resume will also generate a resume event, the event
handler will find the port in resume state, believe it's a device
initated and act accordingly.
2. A port status request might cut the 20ms resume signalling short if a
get_port_status request is handled during the 20ms host resume.
The port will be found in resume state. The timestamp is not set leading
to time_after_eq(jiffoes, timestamp) returning true, as timestamp = 0.
get_port_status will proceed with moving the port to U0.
3. If an error, or anything else happends to the port during device
initated 20ms resume signalling it will leave all device resume
parameters hanging uncleared preventing further resume.
Fix this by using the existing resuming_ports bitfield to indicate if
resume signalling timing is taken care of.
Also check if the resume_done[port] is set before using it in time
comparison. Also clear out any resume signalling related variables if port
is not in U0 or Resume state.
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci-hub.c | 38 --
drivers/usb/host/xhci-ring.c | 3 ++-
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 78241b5..c4f5e41 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -616,8 +616,30 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
if ((raw_port_status & PORT_RESET) ||
!(raw_port_status & PORT_PE))
return 0x;
- if (time_after_eq(jiffies,
- bus_state->resume_done[wIndex])) {
+ /* did port event handler already start 20ms resume timing? */
+ if (!bus_state->resume_done[wIndex]) {
+ /* If not, maybe we are in a host initated resume? */
+ if (test_bit(wIndex, &bus_state->resuming_ports)) {
+ /* Host initated resume doesn't time the resume
+* signalling using resume_done[].
+* It manually sets RESUME state, sleeps 20ms
+* and sets U0 state. This should probably be
+* changed, but not right now, do nothing
+*/
+ } else {
+ /* port resume was discovered now and here,
+* start resume timing
+*/
+ unsigned long timeout = jiffies +
+ msecs_to_jiffies(USB_RESUME_TIMEOUT);
+
+ set_bit(wIndex, &bus_state->resuming_ports);
+ bus_state->resume_done[wIndex] = timeout;
+ mod_timer(&hcd->rh_timer, timeout);
+ }
+ /* Has resume been signalled for 20ms? yet? */
+ } else if (time_after_eq(jiffies,
+bus_state->resume_done[wIndex])) {
int time_left;
xhci_dbg(xhci, "Resume USB2 port %d\n",
@@ -665,6 +687,16 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
status |= USB_PORT_STAT_SUSPEND;
}
}
+ /* Clear stale usb2 resume signalling variables in case port changed
+* state during 20ms resume signalling. For example on error
+*/
+ if ((bus_state->resume_done[wIndex] ||
+test_bit(wIndex, &bus_state->resuming_ports) &&
+(raw_port_status & PORT_PLS_MASK) != XDEV_U3 &&
+(raw_port_status & PORT_PLS_MASK) != XDEV_RESUME)) {
+ bus_state->resume_done[wIndex] = 0;
+ clear_bit(wIndex, &bus_state->resuming_ports);
+ }
if ((raw_port_status & PORT_PLS_MASK) == XDEV_U0
&& (raw_port_status & PORT_POWER)
&& (bus_state->suspended_ports & (1 << wIndex))) {
@@ -995,6 +1027,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16
wValue,
if ((temp & PORT_PE) == 0)
goto error;
+
[PATCH] Minor improvement for smsc95xx netusb driver performance.
Reduce number of memcpy's by 1-2 improve transmit performance by 2-4%,
or reduce CPU usage on a comparable value.
Signed-off-by: Ameen Ali
---
drivers/net/usb/smsc95xx.c | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 66b3ab9..2b59bb7 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1830,7 +1830,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet
*dev,
{
bool csum = skb->ip_summed == CHECKSUM_PARTIAL;
int overhead = csum ? SMSC95XX_TX_OVERHEAD_CSUM : SMSC95XX_TX_OVERHEAD;
- u32 tx_cmd_a, tx_cmd_b;
+ struct tx_commands_t {
+ u32 cmd_a, cmd_b, csum_preamble;
+ }tx_cmds;
/* We do not advertise SG, so skbs should be already linearized */
BUG_ON(skb_shinfo(skb)->nr_frags);
@@ -1855,26 +1857,24 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet
*dev,
+ skb->csum_offset)) = csum_fold(calc);
csum = false;
+ overhead = SMSC95XX_TX_OVERHEAD;
} else {
- u32 csum_preamble = smsc95xx_calc_csum_preamble(skb);
+ tx_cmds.csum_preamble =
smsc95xx_calc_csum_preamble(skb);
skb_push(skb, 4);
- cpu_to_le32s(&csum_preamble);
- memcpy(skb->data, &csum_preamble, 4);
+ cpu_to_le32s(&tx_cmds.csum_preamble);
}
}
- skb_push(skb, 4);
- tx_cmd_b = (u32)(skb->len - 4);
- if (csum)
- tx_cmd_b |= TX_CMD_B_CSUM_ENABLE;
- cpu_to_le32s(&tx_cmd_b);
- memcpy(skb->data, &tx_cmd_b, 4);
-
- skb_push(skb, 4);
- tx_cmd_a = (u32)(skb->len - 8) | TX_CMD_A_FIRST_SEG_ |
+ tx_cmds.cmd_a = (u32)skb->len | TX_CMD_A_FIRST_SEG_ |
TX_CMD_A_LAST_SEG_;
- cpu_to_le32s(&tx_cmd_a);
- memcpy(skb->data, &tx_cmd_a, 4);
+ cpu_to_le32s(&tx_cmds.cmd_a);
+ tx_cmds.cmd_b = (u32)skb->len;
+ if (csum)
+ tx_cmds.cmd_b |= TX_CMD_B_CSUM_ENABLE;
+ cpu_to_le32s(&tx_cmds.cmd_b);
+
+ skb_push(skb, 8);
+ memcpy(skb->data, &tx_cmds, overhead);
return skb;
}
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xhci: Fix memory leak in xhci_pme_acpi_rtd3_enable()
On 27.11.2015 15:24, Mika Westerberg wrote: There is a memory leak because acpi_evaluate_dsm() actually returns an object which the caller is supposed to release. Fix this by calling ACPI_FREE() for the returned object (this expands to kfree() so passing NULL there is fine as well). While there correct indentation in !CONFIG_ACPI case. Signed-off-by: Mika Westerberg Thanks, I'll add it to the queue -Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: gadget: ether: Allow changing the MTU
The gadget ethernet driver supports changing the MTU, but only allows this
when the USB cable is removed. The comment indicates that this is because
the "peer won't know". Even if the network link is still down and only the
USB link is established, the driver won't allow the change.
Other network interfaces allow changing the MTU any time, and don't force
the link to be disabled. This makes perfect sense, because in order to be
able to negotiate the MTU, the link needs to be up.
Remove the restriction so that it is now actually possible to change the
MTU (e.g. using "ifconfig usb0 mtu 15000") without having to manually pull
the plug or change the driver's default setting.
This is especially important after commit bba787a860fa
("usb: gadget: ether: Allow jumbo frames")
Signed-off-by: Mike Looijmans
---
v2: Fix commit reference (checkpatch) and unused variable 'dev' (kbuild test
robot)
drivers/usb/gadget/function/u_ether.c | 18 --
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/gadget/function/u_ether.c
b/drivers/usb/gadget/function/u_ether.c
index 6554322..637809e 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -143,21 +143,11 @@ static inline int qlen(struct usb_gadget *gadget,
unsigned qmult)
static int ueth_change_mtu(struct net_device *net, int new_mtu)
{
- struct eth_dev *dev = netdev_priv(net);
- unsigned long flags;
- int status = 0;
+ if (new_mtu <= ETH_HLEN || new_mtu > GETHER_MAX_ETH_FRAME_LEN)
+ return -ERANGE;
+ net->mtu = new_mtu;
- /* don't change MTU on "live" link (peer won't know) */
- spin_lock_irqsave(&dev->lock, flags);
- if (dev->port_usb)
- status = -EBUSY;
- else if (new_mtu <= ETH_HLEN || new_mtu > GETHER_MAX_ETH_FRAME_LEN)
- status = -ERANGE;
- else
- net->mtu = new_mtu;
- spin_unlock_irqrestore(&dev->lock, flags);
-
- return status;
+ return 0;
}
static void eth_get_drvinfo(struct net_device *net, struct ethtool_drvinfo *p)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Infrastructure for zerocopy I/O
On Fri, 2015-11-27 at 00:04 +0100, Steinar H. Gunderson wrote:
> On Thu, Nov 26, 2015 at 01:26:32AM +0100, Steinar H. Gunderson wrote:
> >> There are numerous smaller issues: The allocated memory needs to be
> >> charged against usbfs_memory_usage
> > I'll fix this.
>
> Fixed in updated patch (attached).
>
> > I'll fix this, too.
>
> Also fixed.
>
> Now also checkpatch clean.
>
> /* Steinar */
+static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
+{
+ struct usb_memory *usbm = NULL;
+ struct usb_dev_state *ps = file->private_data;
+ size_t size = vma->vm_end - vma->vm_start;
+ int ret;
+
+ ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
+ if (ret)
+ return ret;
+
+ usbm = alloc_usb_memory(ps, size);
+ if (!usbm)
+ return -ENOMEM;
Looks like an accounting leak in the error case.
+
+ if (remap_pfn_range(vma, vma->vm_start,
+ virt_to_phys(usbm->mem) >> PAGE_SHIFT,
+ size, vma->vm_page_prot) < 0)
+ return -EAGAIN;
The same leak and a memory leak.
HTH
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: ether: Allow changing the MTU
Hi Mike,
[auto build test WARNING on: balbi-usb/next]
[also build test WARNING on: v4.4-rc3 next-20151127]
url:
https://github.com/0day-ci/linux/commits/Mike-Looijmans/usb-gadget-ether-Allow-changing-the-MTU/20151130-162847
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
config: i386-randconfig-s1-201548 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers/usb/gadget/function/u_ether.c: In function 'ueth_change_mtu':
>> drivers/usb/gadget/function/u_ether.c:146:18: warning: unused variable 'dev'
>> [-Wunused-variable]
struct eth_dev *dev = netdev_priv(net);
^
vim +/dev +146 drivers/usb/gadget/function/u_ether.c
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
130 #else
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
131 #define VDBG(dev, fmt, args...) \
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
132do { } while (0)
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
133 #endif /* DEBUG */
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
134
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
135 #define ERROR(dev, fmt, args...) \
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
136xprintk(dev , KERN_ERR , fmt , ## args)
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
137 #define INFO(dev, fmt, args...) \
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
138xprintk(dev , KERN_INFO , fmt , ## args)
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
139
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
140
/*-*/
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
141
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
142 /* NETWORK DRIVER HOOKUP (to the layer above this driver) */
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
143
ccad637b drivers/usb/gadget/u_ether.c Stephen Hemminger 2008-11-19
144 static int ueth_change_mtu(struct net_device *net, int new_mtu)
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
145 {
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
@146struct eth_dev *dev = netdev_priv(net);
1616c4ef drivers/usb/gadget/function/u_ether.c Mike Looijmans2015-11-30
147
1616c4ef drivers/usb/gadget/function/u_ether.c Mike Looijmans2015-11-30
148if (new_mtu <= ETH_HLEN || new_mtu > GETHER_MAX_ETH_FRAME_LEN)
1616c4ef drivers/usb/gadget/function/u_ether.c Mike Looijmans2015-11-30
149return -ERANGE;
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
150net->mtu = new_mtu;
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
151
1616c4ef drivers/usb/gadget/function/u_ether.c Mike Looijmans2015-11-30
152return 0;
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
153 }
2b3d942c drivers/usb/gadget/u_ether.c David Brownell2008-06-19
154
:: The code at line 146 was first introduced by commit
:: 2b3d942c4878084a37991a65e66512c02b8fa2ad usb ethernet gadget: split out
network core
:: TO: David Brownell
:: CC: Greg Kroah-Hartman
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: Binary data
Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
On Fri, 2015-11-27 at 18:39 -0800, Greg KH wrote: > Yes, it's tough, I know, good luck. > > Also deal with multiple devices, busses that are ordered differently > depending on the phase of the moon, and other fun things with dynamic > devices and ioctls. It's a loosing battle :) IMHO the fundamental problem is using one device node per device. And I think it should be tackled. Yet I have no idea how to do this in a compatible manner. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: uvc: fix permissions of configfs attributes
Hi Mian,
Thank you for the patch.
On Saturday 28 November 2015 18:35:44 Mian Yousaf Kaukab wrote:
> 76e0da3 "usb-gadget/uvc: use per-attribute show and store methods"
> removed write permission for writeable attributes. Correct attribute
> permissions.
>
> Fixes: 76e0da3 "usb-gadget/uvc: use per-attribute show and store methods"
> Signed-off-by: Mian Yousaf Kaukab
This looks good to me.
Reviewed-by: Laurent Pinchart
> ---
> drivers/usb/gadget/function/uvc_configfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c index 289ebca..ad8c9b0 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -20,7 +20,7 @@
> #define UVC_ATTR(prefix, cname, aname) \
> static struct configfs_attribute prefix##attr_##cname = { \
> .ca_name= __stringify(aname), \
> - .ca_mode= S_IRUGO, \
> + .ca_mode= S_IRUGO | S_IWUGO,\
> .ca_owner = THIS_MODULE, \
> .show = prefix##cname##_show, \
> .store = prefix##cname##_store,\
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: host: pci_quirks: fix memory leak, by adding iounmap
On 30.11.2015 08:28, Saurabh Sengar wrote: pinging in case this patch is lost. On 6 November 2015 at 17:46, Saurabh Sengar wrote: added iounmap inorder to free memory mapped to base before returning Signed-off-by: Saurabh Sengar --- drivers/usb/host/pci-quirks.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c index f940056..332f687 100644 --- a/drivers/usb/host/pci-quirks.c +++ b/drivers/usb/host/pci-quirks.c @@ -990,7 +990,7 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev) /* We're reading garbage from the controller */ dev_warn(&pdev->dev, "xHCI controller failing to respond"); - return; + goto iounmap; } if (!ext_cap_offset) @@ -1061,7 +1061,7 @@ hc_init: "xHCI HW did not halt within %d usec status = 0x%x\n", XHCI_MAX_HALT_USEC, val); } - +iounmap: iounmap(base); } -- 1.9.1 Thanks, I'll add it send it forward soon -Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: uvc: fix permissions of configfs attributes
Hi Mian, sorry for the bug, the fix looks correct. Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: ether: Allow changing the MTU
The gadget ethernet driver supports changing the MTU, but only allows this
when the USB cable is removed. The comment indicates that this is because
the "peer won't know". Even if the network link is still down and only the
USB link is established, the driver won't allow the change.
Other network interfaces allow changing the MTU any time, and don't force
the link to be disabled. This makes perfect sense, because in order to be
able to negotiate the MTU, the link needs to be up.
Remove the restriction so that it is now actually possible to change the
MTU (e.g. using "ifconfig usb0 mtu 15000") without having to manually pull
the plug or change the driver's default setting.
This is especially important after commit bba787a860fa:
"usb: gadget: ether: Allow jumbo frames".
Signed-off-by: Mike Looijmans
---
drivers/usb/gadget/function/u_ether.c | 16
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/gadget/function/u_ether.c
b/drivers/usb/gadget/function/u_ether.c
index 6554322..9c60e57 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -144,20 +144,12 @@ static inline int qlen(struct usb_gadget *gadget,
unsigned qmult)
static int ueth_change_mtu(struct net_device *net, int new_mtu)
{
struct eth_dev *dev = netdev_priv(net);
- unsigned long flags;
- int status = 0;
- /* don't change MTU on "live" link (peer won't know) */
- spin_lock_irqsave(&dev->lock, flags);
- if (dev->port_usb)
- status = -EBUSY;
- else if (new_mtu <= ETH_HLEN || new_mtu > GETHER_MAX_ETH_FRAME_LEN)
- status = -ERANGE;
- else
- net->mtu = new_mtu;
- spin_unlock_irqrestore(&dev->lock, flags);
+ if (new_mtu <= ETH_HLEN || new_mtu > GETHER_MAX_ETH_FRAME_LEN)
+ return -ERANGE;
+ net->mtu = new_mtu;
- return status;
+ return 0;
}
static void eth_get_drvinfo(struct net_device *net, struct ethtool_drvinfo *p)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 00/14] Equivalent of tcm_usb_gadget with configfs
On 11/29/2015 07:16 AM, Nicholas A. Bellinger wrote: > Ping (2) on this series to Joel + Sebastian. ;) > > If there aren't any immediate objections on the fs/configfs/ side from > JLBEC, I'll assume this series can be added as target-pending/for-next > WIP code this week, yes..? Yes, please. If there is anything then it will be fixed up later… > > Thank you, > > --nab > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
