from:"Krzysztof Opasiak"




On 06.04.2018 15:03, Felipe Balbi wrote:
[...]

okay, but we don't... Do you wanna send some patches?


another thing to keep in mind is that we're trying to be a USB-compliant
stack. Gadget Framework was never meant to be a testing ground for the
Host Stack. There are VERY flexible tools designed specifically for that
in market.


True but we still have GadgetFS and FunctionFS and no one (apart from 
the vendor who can sign his platform image) can guarantee that userspace 
that uses those interfaces is USB compliant...




Maybe, companies who are investing in Fuzzying, should just invest in
one of those tools instead of rellying on a stack that WANTS to send
compliant USB-packets at all times?



Hmmm but apart from companies there are tons of hobbyist or students 
(like mine;)) who are trying to build useful tools using cheap and 
popular hardware. Take USBProxy[1] or GadgetFS backend for umap2[2] as 
an example.


Footnotes:
1 - https://github.com/dominicgs/USBProxy
2 - https://github.com/nccgroup/umap2

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: Handling custom device control requests in userspace




On 06.04.2018 14:35, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:

On 06.04.2018 12:18, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:




A few advantages over a couple options I've considered are that this mostly
reuses existing functionalities and won't affect users that haven't enabled
it. Please let me know of any feedback on the design or any possible
implementation issues.


IMHO, considering the amount of Android users, we can merge this into
composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
something along the lines of

if (IS_ENABLED(CONFIG_ANDROID))
android_audio_accessory_init();

should get the job done.


Huh and what with people that are not android but need to support
Android accessory in their products?


Do those really exist?

Well, if they _really_ exist, we can add a "Enable Support for Android
Audio Accessory" specific to the gadget framework. Then test for:

if (IS_ENABLED(CONFIG_USB_GADGET_ANDROID_AUDIO_ACCESSORY))

or whatever. I'm not certain such devices exist though, need
confirmation.



Yes, they exist. Most of Tizen phones support android accessory.


Fair enough. A specific Kconfig for that is okay.



Let me just mention that if we had a generic interface we could cover 
not only android requirements but also do a great job for people who are 
fuzzing USB hosts. Such an interface would allow to easily craft some 
random requests without all this noise related to using GadgetFS...


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: Handling custom device control requests in userspace




On 06.04.2018 12:18, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:




A few advantages over a couple options I've considered are that this mostly
reuses existing functionalities and won't affect users that haven't enabled
it. Please let me know of any feedback on the design or any possible
implementation issues.


IMHO, considering the amount of Android users, we can merge this into
composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
something along the lines of

if (IS_ENABLED(CONFIG_ANDROID))
android_audio_accessory_init();

should get the job done.


Huh and what with people that are not android but need to support
Android accessory in their products?


Do those really exist?

Well, if they _really_ exist, we can add a "Enable Support for Android
Audio Accessory" specific to the gadget framework. Then test for:

if (IS_ENABLED(CONFIG_USB_GADGET_ANDROID_AUDIO_ACCESSORY))

or whatever. I'm not certain such devices exist though, need
confirmation.



Yes, they exist. Most of Tizen phones support android accessory.

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: Handling custom device control requests in userspace




On 06.04.2018 12:12, Felipe Balbi wrote:


Hi Jerry,

Jerry Zhang  writes:

Hi all,

I've been looking for a way to handle custom device targeted control
requests from userspace. This would allow us to move away from needing
kernel patches to implement
https://source.android.com/devices/accessories/aoa. It seems like we are
close to being able to do this. With the flag FUNCTIONFS_ALL_CTRL_RECIP, an
f_fs function can handle all requests (device, interface, etc.) that are
not otherwise claimed, and functionfs already implements a pretty good
interface for userspace control requests through ep0. The issue is that
currently the function must be in the config in order to handle requests.
However, at any point in time we don't know which functionfs function is in
the config, or even that the config contains any ffs functions at all.

Here are my (early) thoughts on how to implement this feature. The goal is
to allow a function not in a configuration to handle control requests.

- Each configfs gadget contains a special config_desc called
"control_config". This acts as a normal config_desc except it is not added
back to cdev and instead is a new field in struct gadget_info.

- Functions can be linked into control_config. This is the same as a normal
usb_cfg_link. Functions linked this way can't be used in a normal config
since each function only has one list item.

- On configfs_bind, all functions in control_config are also bound. On
configfs_unbind, all functions in control_config are also unbound. Because
they aren't in cdev they don't appear in descriptors.

- Configfs will now have configfs_setup, which first attempts
composite_setup. If that fails, it will go through functions in
control_config and do the normal req_match / setup procedure. Since each
function here is bound and has a reference to cdev, it can successfully
match and handle control requests.

Thus a user that wants to handle all control requests can make a functionfs
instance "ctrlf" with dummy descriptors, and include the flag
ALL_CTRL_RECIP. Then they can link functions/ffs.ctrlf g1/control_config/f1
and handle control requests at /dev/usb-ffs/ctrlf/ep0.

A few advantages over a couple options I've considered are that this mostly
reuses existing functionalities and won't affect users that haven't enabled
it. Please let me know of any feedback on the design or any possible
implementation issues.


IMHO, considering the amount of Android users, we can merge this into
composite.c itself. Just make the code depend on CONFIG_ANDROID. Or
something along the lines of

if (IS_ENABLED(CONFIG_ANDROID))
android_audio_accessory_init();

should get the job done.


Huh and what with people that are not android but need to support 
Android accessory in their products?


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

libusbgx v0.2.0 released

2018-03-02 Thread Krzysztof Opasiak


Hello,

This is just a note to let you know that v0.2.0 of libusbg-neXt[1] has 
just been released.


Summary of changes:
- Use prefix for our enums
- Use string names similar to USB spec
- Get rid of static buffers
- Add support for HID function
- Add support for UAC2 function
- Add support for OS descriptors
- Fix C++ compilation issues
- Many smaller fixes

I encourage you to check out new release and contribute!

Footnotes:
1 - https://github.com/libusbgx/libusbgx

Cheers,
--
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] usbip: tools usbip_attach: Fix cryptic error messages

2018-02-27 Thread Krzysztof Opasiak




On 02/27/2018 11:23 PM, Shuah Khan wrote:

Attach device error message is cryptic and useless. Fix it to be
informative.

Signed-off-by: Shuah Khan 
---
  tools/usb/usbip/src/usbip_attach.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/src/usbip_attach.c 
b/tools/usb/usbip/src/usbip_attach.c
index 7f07b2d50f59..f60738735398 100644
--- a/tools/usb/usbip/src/usbip_attach.c
+++ b/tools/usb/usbip/src/usbip_attach.c
@@ -195,7 +195,8 @@ static int attach_device(char *host, char *busid)
  
  	rhport = query_import_device(sockfd, busid);

if (rhport < 0) {
-   err("query");
+   err("Attach request for Device %s. Is this device exported?",
+   busid);
return -1;
}


The code itself is ok and you may put my:

Reviewed-by: Krzysztof Opasiak 

but just because of my curiosity, there is a number of places in usbip 
tools where the same level of descriptiveness is used for error message. 
Why did you choose to fix this one not any other or all others?;)


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: [PATCH] usbip: vudc: fix null pointer dereference on udc->lock

2018-02-27 Thread Krzysztof Opasiak




On 02/26/2018 05:40 PM, Shuah Khan wrote:

On 02/22/2018 10:39 AM, Colin King wrote:

From: Colin Ian King 

Currently the driver attempts to spin lock on udc->lock before a NULL
pointer check is performed on udc, hence there is a potential null
pointer dereference on udc->lock.  Fix this by moving the null check
on udc before the lock occurs.

Fixes: ea6873a45a22 ("usbip: vudc: Add SysFS infrastructure for VUDC")
Signed-off-by: Colin Ian King 
---
  drivers/usb/usbip/vudc_sysfs.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c
index d86f7291..6dcd3ff655c3 100644
--- a/drivers/usb/usbip/vudc_sysfs.c
+++ b/drivers/usb/usbip/vudc_sysfs.c
@@ -105,10 +105,14 @@ static ssize_t usbip_sockfd_store(struct device *dev, 
struct device_attribute *a
if (rv != 0)
return -EINVAL;
  
+	if (!udc) {

+   dev_err(dev, "no device");
+   return -ENODEV;
+   }
spin_lock_irqsave(&udc->lock, flags);
/* Don't export what we don't have */
-   if (!udc || !udc->driver || !udc->pullup) {
-   dev_err(dev, "no device or gadget not bound");
+   if (!udc->driver || !udc->pullup) {
+   dev_err(dev, "gadget not bound");
ret = -ENODEV;
goto unlock;
}



Thanks for the patch. Looks good to me.

Acked-by: Shuah Khan 


Reviewed-by: Krzysztof Opasiak 

but you could fix also a similar bug one one function above 
(dev_desc_read()) ;)


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: Host & gadget transparent driver




On 02/22/2018 11:17 AM, Greg KH wrote:

On Thu, Feb 22, 2018 at 10:42:17AM +0100, Krzysztof Opasiak wrote:



On 02/22/2018 10:32 AM, Ran Shalit wrote:
[...]


I don't know the exact case nor I'm license specialist but I think it's
doable without license violation. Take a look at official displaylink
drivers[1]... They consist of two parts. First one is Kernel GPL code[2]
which is obviously published to not violate the license and second part is
userspace binary blob that receives video stream from this "open source
driver" and then uses libusb to communicate with the device...

Footnotes:
1 - http://www.displaylink.com/downloads/ubuntu
2 - https://github.com/DisplayLink/evdi
Best regards,



Krzysztof , I will check the proxy git .
What's the difference between the proxy you've given before to the DisplayLink?



Displaylink driver is unrelated to the proxy. It's just an example of how to
not violate GPL license and not publish your USB protocol and provide only
binary blob with it;)


Um, do not say "how not to violate GPL" without being a lawyer please.


You are absolutely right and what I wrote above it's only my own 
"feeling" not any lawyer statement.



I wouldn't bet that what they are doing is all so cut-and-dry...


Me neither so I'm only letting know that such things are happening in 
the wild and if you know some specialist you may let him know about 
this;) If it turns out that it's bad and you/LF manage to persuade DL to 
open their driver because of that, then oh man you are a magician for me:)


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: Host & gadget transparent driver




On 02/22/2018 10:32 AM, Ran Shalit wrote:
[...]


I don't know the exact case nor I'm license specialist but I think it's
doable without license violation. Take a look at official displaylink
drivers[1]... They consist of two parts. First one is Kernel GPL code[2]
which is obviously published to not violate the license and second part is
userspace binary blob that receives video stream from this "open source
driver" and then uses libusb to communicate with the device...

Footnotes:
1 - http://www.displaylink.com/downloads/ubuntu
2 - https://github.com/DisplayLink/evdi
Best regards,



Krzysztof , I will check the proxy git .
What's the difference between the proxy you've given before to the DisplayLink?



Displaylink driver is unrelated to the proxy. It's just an example of 
how to not violate GPL license and not publish your USB protocol and 
provide only binary blob with it;)


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: Host & gadget transparent driver




On 02/22/2018 07:53 AM, Greg KH wrote:

On Wed, Feb 21, 2018 at 11:23:26PM +0200, Ran Shalit wrote:

Hello,

I am facing the following challenge:

We have a camera device, and a ready drivers in the following configuration:

(1) host <--> camera

The drivers for host is a binary, i.e. source code is probably not
available, and also the protocol datasheet is probably not available.


Really?  A USB host driver that is not released under the GPL?  That's
really difficult to imagine, but you are on your own here as that's an
obvious license violation.  Please go talk to your lawyers about this
problem, or you will have bigger problems if you try to rely on this.



I don't know the exact case nor I'm license specialist but I think it's 
doable without license violation. Take a look at official displaylink 
drivers[1]... They consist of two parts. First one is Kernel GPL code[2] 
which is obviously published to not violate the license and second part 
is userspace binary blob that receives video stream from this "open 
source driver" and then uses libusb to communicate with the device...


Footnotes:
1 - http://www.displaylink.com/downloads/ubuntu
2 - https://github.com/DisplayLink/evdi
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: Host & gadget transparent driver




On 02/21/2018 10:23 PM, Ran Shalit wrote:

Hello,

I am facing the following challenge:

We have a camera device, and a ready drivers in the following configuration:

(1) host <--> camera

The drivers for host is a binary, i.e. source code is probably not
available, and also the protocol datasheet is probably not available.

But we need to use the camera in the following configuration:

(2) host <---> embedded board  <--> camera

In other words, there is a transparent medium in between the host and device.

I would like to ask what is the general concept for implementing the
drivers in the embedded board ?

Does these task requires reverse engineering ?

Is there any known example or driver which does something similar ?


I use to do such sick things like this. It can be easily done using 
USBProxy[1] (libusb + gadgetfs) but I'm not sure if it supports iso 
transfers which I assume is required for your camera...


Footnotes:
1 - https://github.com/dominicgs/USBProxy

--
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: USB gadget : generic functionfs function has no os_desc while rndis function has, why?

2018-02-15 Thread Krzysztof Opasiak




On 02/11/2018 08:52 AM, Jun Sun wrote:

Thanks for your reply, Krzysztof.  I now understand all the fields.

However, I just realize I can't really extend your example to cover my
need, because your example uses RNDIS where kernel already support OS
descriptor.

In my case I'm preparing my own functionfs-based gadget.  I suppose
somewhere inside my functionfs daemon, I should add OS descriptor
support.  Is that understanding right?  If so, is there any example?

I searched around internet and can't seem to find a single example
where functionfs-based USB gadget supports OS descriptor.  In fact, it
is not even clear to me what kernel interfaces are used for conveying
OS descriptor for functionfs-based function.

Thanks again.  Really appreciate your help.


I don't have any good example for that but this should be simple.
It's only adding an additional flag to you descriptors (FFS_HAS_OS_DESC 
or sth similar, check ffs header for this) and then adding your os_desc 
to your descriptors.


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: USB gadget : generic functionfs function has no os_desc while rndis function has, why?

2018-02-07 Thread Krzysztof Opasiak




On 02/07/2018 08:25 AM, Jun Sun wrote:

Thanks, Krysztof.

I'm trying to follow your example and maybe to massage it into my need.

One quick question -

In your example, you set vendor code to be 0xBC.  Is this code of any
significance?  What values should I use?

b_vendor_code = 0xBC,


Well... according to "spec"[1]:

"bMS_VendorCode is used to retrieve the associated feature descriptors. 
The vendor defines the format of this field. For further details, see 
“OS Feature Descriptors” later in this paper."


USB complete[2] elaborates this pretty good. But to be totally honest I 
took this value from 3.


So from your perspective this could be any value other than 0;)

Footnotes:
1 - 
https://docs.microsoft.com/en-us/windows-hardware/drivers/network/mb-interface-model-supplement


2 - 
https://books.google.pl/books?id=pkefBgAAQBAJ&pg=PA347&lpg=PA347&dq=bMS_VendorCode+rndis&source=bl&ots=6_hwlTDvwy&sig=WmjSiXX9qvWF5aLBxa8M7xYvDNM&hl=pl&sa=X&ved=0ahUKEwj1kcXzvJPZAhVK16QKHeVuAOMQ6AEIRDAE#v=onepage&q=bMS_VendorCode%20rndis&f=false


3 - http://doc.hcc-embedded.com/download/attachments/27590911/HCC RNDIS 
Device Class Driver Windows Automatic Installation Guide 
v1.10.pdf?version=1&modificationDate=1507279811000&api=v2


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: [PATCH] usb: gadget: f_uac2: fix bFirstInterface in composite gadget

2018-01-15 Thread Krzysztof Opasiak




On 01/12/2018 07:43 PM, John Keeping wrote:

If there are multiple functions associated with a configuration, then
the UAC2 interfaces may not start at zero.  Set the correct first
interface number in the association descriptor so that the audio
interfaces are enumerated correctly in this case.

Signed-off-by: John Keeping 
---
  drivers/usb/gadget/function/f_uac2.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/function/f_uac2.c 
b/drivers/usb/gadget/function/f_uac2.c
index 11fe788b4308..d2dc1f00180b 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -524,6 +524,8 @@ afunc_bind(struct usb_configuration *cfg, struct 
usb_function *fn)
dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
return ret;
}
+   iad_desc.bFirstInterface = ret;
+
std_ac_if_desc.bInterfaceNumber = ret;
uac2->ac_intf = ret;
uac2->ac_alt = 0;


Reviewed-by: Krzysztof Opasiak 

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: USB gadget : generic functionfs function has no os_desc while rndis function has, why?

2018-01-04 Thread Krzysztof Opasiak




On 01/04/2018 06:58 AM, Jun Sun wrote:

Hi, all,

I'm playing with USB gadget with configfs on raspberry pi zero w.  My
goal is to setup a generic functionfs function that uses Windows OS
descriptor so that windows would automatically install winusb driver.
It seems I would have to

- enable MS-specific os descriptor
- specify the compatiblity id to be "WINUSB"
- specify interface GUID (optional?)

It seems simple enough.


You can checkout libusbgx example for a "good" values for RNDIS and 
expected directory structure:


https://github.com/libusbgx/libusbgx/blob/master/examples/gadget-rndis-os-desc.c






However, the first problem I'm having is that I can't find the
os_desc/ directory for the ffs function I created. 


Because there is no such directory;)
os_desc in case of functionfs are provided by daemon together with USB 
descriptors.


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: [PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-13 Thread Krzysztof Opasiak




On 12/13/2017 10:29 AM, Felipe Balbi wrote:


Hi,

Alan Stern  writes:

Krzysztof Opasiak  writes:

On 12/12/2017 01:31 PM, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:

Every function should have a type and instance name.
Unfortunately in most cases instance name was left unused and unchecked.
This may lead to situations like FunctionFS device name identified by ""
or some misleading debug messages from TCM like:

tcm: Activating

To avoid this let's add a check that instance name should have at least
one character.

Reported-by: Stefan Agner 
Signed-off-by: Krzysztof Opasiak 
---
   drivers/usb/gadget/configfs.c | 5 +
   1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index aeb9f3c40521..bdc9ec597d6a 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -548,6 +548,11 @@ static struct config_group *function_make(
*instance_name = '\0';
instance_name++;
   
+	if (*instance_name == '\0') {

+   pr_err("Instance name (after .) should not be empty\n");
+   return ERR_PTR(-EINVAL);
+   }


aand just like that you break potentially existing scripts :-)

We need to find a better way of enforcing a name which doesn't break
existing users.


I'm really open for suggestions how to enforce this without breaking
those scripts ;)

The origin of this commit is github issue for libusbgx[1].
So the problem is that library allows to create a function with empty
name (because I mistakenly assumed that kernel rejects this) but then it
is unable to reinitialize the ConfigFS state because there is a check
that disallows this. From my point of view I'd be happy to disallow
empty names because it causes some problems (f_fs) or weird debug
messages (f_tcm) so is generally inconvenient and seems to be
unintentional. But I would like to keep this consistent with kernel policy.


I think we need to first fix libusbgx to prevent empty names.

I don't want to be the one hearing from Linus that "we don't break
userspace". It's clear that empty names shouldn't be allowed, but they
_are_ allowed as of today, so how can we cause a regression all of a
sudden?

Alan, Greg, any suggestions?


You could do some silly name munging, like changing an empty name to
" " whenever you encounter it.  Or adding an '_' to the end of any name
that consists of nothing but '_' characters.


Hmm, that could be done. So everytime userspace gives us an empty name,
we would convert to '_'. That still doesn't solve the problems of
mounting functionfs, though. But I guess there's nothing that can be
done in that case.



How is it different from disallowing empty name?
It may also cause some "broken" scripts stop working.
Isn't it going to introduce some weird problems like:

mkdir g1/function/ffs._
mkdir g2/function/ffs.
-EBUSY

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: [PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-12 Thread Krzysztof Opasiak




On 12/12/2017 02:16 PM, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:

On 12/12/2017 01:31 PM, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:

Every function should have a type and instance name.
Unfortunately in most cases instance name was left unused and unchecked.
This may lead to situations like FunctionFS device name identified by ""
or some misleading debug messages from TCM like:

tcm: Activating

To avoid this let's add a check that instance name should have at least
one character.

Reported-by: Stefan Agner 
Signed-off-by: Krzysztof Opasiak 
---
   drivers/usb/gadget/configfs.c | 5 +
   1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index aeb9f3c40521..bdc9ec597d6a 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -548,6 +548,11 @@ static struct config_group *function_make(
*instance_name = '\0';
instance_name++;
   
+	if (*instance_name == '\0') {

+   pr_err("Instance name (after .) should not be empty\n");
+   return ERR_PTR(-EINVAL);
+   }


aand just like that you break potentially existing scripts :-)

We need to find a better way of enforcing a name which doesn't break
existing users.


I'm really open for suggestions how to enforce this without breaking
those scripts ;)

The origin of this commit is github issue for libusbgx[1].
So the problem is that library allows to create a function with empty
name (because I mistakenly assumed that kernel rejects this) but then it
is unable to reinitialize the ConfigFS state because there is a check
that disallows this. From my point of view I'd be happy to disallow
empty names because it causes some problems (f_fs) or weird debug
messages (f_tcm) so is generally inconvenient and seems to be
unintentional. But I would like to keep this consistent with kernel policy.


I think we need to first fix libusbgx to prevent empty names.


I created PR for this[1]. If anyone here has any objections to this 
please let me now as soon as possible.


If there is no veto or other solution, I merge this in the end of week.

Footnotes:
1 - https://github.com/libusbgx/libusbgx/pull/20

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: [PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-12 Thread Krzysztof Opasiak




On 12/12/2017 01:31 PM, Felipe Balbi wrote:


Hi,

Krzysztof Opasiak  writes:

Every function should have a type and instance name.
Unfortunately in most cases instance name was left unused and unchecked.
This may lead to situations like FunctionFS device name identified by ""
or some misleading debug messages from TCM like:

tcm: Activating

To avoid this let's add a check that instance name should have at least
one character.

Reported-by: Stefan Agner 
Signed-off-by: Krzysztof Opasiak 
---
  drivers/usb/gadget/configfs.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index aeb9f3c40521..bdc9ec597d6a 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -548,6 +548,11 @@ static struct config_group *function_make(
*instance_name = '\0';
instance_name++;
  
+	if (*instance_name == '\0') {

+   pr_err("Instance name (after .) should not be empty\n");
+   return ERR_PTR(-EINVAL);
+   }


aand just like that you break potentially existing scripts :-)

We need to find a better way of enforcing a name which doesn't break
existing users.


I'm really open for suggestions how to enforce this without breaking 
those scripts ;)


The origin of this commit is github issue for libusbgx[1].
So the problem is that library allows to create a function with empty 
name (because I mistakenly assumed that kernel rejects this) but then it 
is unable to reinitialize the ConfigFS state because there is a check 
that disallows this. From my point of view I'd be happy to disallow 
empty names because it causes some problems (f_fs) or weird debug 
messages (f_tcm) so is generally inconvenient and seems to be 
unintentional. But I would like to keep this consistent with kernel policy.


Footnotes:
1 - https://github.com/libusbgx/libusbgx/issues/19
--
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

[PATCH] usb: gadget: configfs: Disallow empty function instance name

2017-12-12 Thread Krzysztof Opasiak

Every function should have a type and instance name.
Unfortunately in most cases instance name was left unused and unchecked.
This may lead to situations like FunctionFS device name identified by ""
or some misleading debug messages from TCM like:

tcm: Activating

To avoid this let's add a check that instance name should have at least
one character.

Reported-by: Stefan Agner 
Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/configfs.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index aeb9f3c40521..bdc9ec597d6a 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -548,6 +548,11 @@ static struct config_group *function_make(
*instance_name = '\0';
instance_name++;
 
+   if (*instance_name == '\0') {
+   pr_err("Instance name (after .) should not be empty\n");
+   return ERR_PTR(-EINVAL);
+   }
+
fi = usb_get_function_instance(func_name);
if (IS_ERR(fi))
return ERR_CAST(fi);
-- 
2.9.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: usb device implemented with functionfs - must app run as root?

2017-12-06 Thread Krzysztof Opasiak




On 12/06/2017 07:26 AM, Greg KH wrote:

On Tue, Dec 05, 2017 at 10:09:35PM +, andy_purc...@keysight.com wrote:

I have implemented a USB device using functionfs.
A colleague now says our app must run as a normal user, not as root.

I tried it and it does not work.
The problem is this - the endpoint files created by the OS are owned by root.
These ep files are created after I write the descriptors and strings to the 
/dev/usbffs/ep0 file.

$ ls -l /dev/usbffs/
total 0
-rw-rw-rw- 1 xyzuser xyzgrp 0 Dec  5 21:36 ep0
-rw--- 1 rootroot   0 Dec  5 21:39 ep1
-rw--- 1 rootroot   0 Dec  5 21:39 ep2
-rw--- 1 rootroot   0 Dec  5 21:39 ep3

A normal user-space app cannot open, write, read, these ep files.

Is there a remedy for this?


Write a udev rule to change the owners of those files :)

You must have done that already for the ep0 file, right?



FunctionFS is a separate file system not a group of device nodes it's 
just mounted under /dev/usbffs. So technically epX are not device nodes 
and as far as I know (please correct me if I'm wrong) there is no uevent 
then epX is created.


Can we use udev for a custom files other than device nodes? Isn't it 
only uevent parser?
Not to mention about race condition between service opening the file and 
udev trying to execute the rule;)


I'm not sure if you use systemd or not but there is a FunctionFS based 
activation and this is how we solve this problem. systemd is running as 
a root and opens all epX files and pass them to the service which then 
can run with lower privileges. Additional benefit is that systemd 
doesn't close those fds so even if your demon crashes whole gadget is 
not going away, all other functions are still usable.


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: Multiple g_zero gadgets

2017-11-13 Thread Krzysztof Opasiak




On 11/13/2017 10:53 AM, Krzysztof Opasiak wrote:



On 11/09/2017 12:15 PM, Felipe Balbi wrote:


Hi,

Hinko Kocevar  writes:

The way dummy was written, it can only instantiate one gadget. You
either need a real USB peripheral controller, or you need to patch 
dummy

to instantiate more than one gadget.

--
balbi


By dummy - are you referring to g_zero of dummy_hcd?


Okay, we need a little more explanation. In order to have a gadget
driver like g_zero loaded, you need a USB peripheral controller (in your
case, that's handled by dummy).

Now, when you want that g_zero to be useful, you need to plug it to a
USB host controller (in your case, that's also handled by dummy).

What I'm saying, though, is that this dummy (dummy_hcd.c) can only
handle ONE gadget (a fake USB peripheral controller) at a time. It does
NOT instantiate more than one of those. Because of that, there's no way
to get g_zero to probe for a second time.

In fact, even if you were to patch dummy_hcd.c to instantiate N gadgets,


MODULE_PARM_DESC(num, "number of emulated controllers");

If you set this to 5 you will get 5 pairs of UDC and HCD.


you wouldn't be able to use g_zero.ko, and would be required to rely on
our configfs interface for dynamically generating and binding different
gadget drivers to each of your gadget instances, but that's another
story altogether.


Then you can use configfs and instantiate more then one g_zero 
equivalent and bind it do dummy_udc.0, dummy_udc.1 etc.


Each of them will be connected to a separate bus but they all will be 
visible in the system.


Gr
thunderbird messed up threading for this topic.
Sorry for repeating after Alan.

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: Multiple g_zero gadgets

2017-11-13 Thread Krzysztof Opasiak




On 11/09/2017 12:15 PM, Felipe Balbi wrote:


Hi,

Hinko Kocevar  writes:

The way dummy was written, it can only instantiate one gadget. You
either need a real USB peripheral controller, or you need to patch dummy
to instantiate more than one gadget.

--
balbi


By dummy - are you referring to g_zero of dummy_hcd?


Okay, we need a little more explanation. In order to have a gadget
driver like g_zero loaded, you need a USB peripheral controller (in your
case, that's handled by dummy).

Now, when you want that g_zero to be useful, you need to plug it to a
USB host controller (in your case, that's also handled by dummy).

What I'm saying, though, is that this dummy (dummy_hcd.c) can only
handle ONE gadget (a fake USB peripheral controller) at a time. It does
NOT instantiate more than one of those. Because of that, there's no way
to get g_zero to probe for a second time.

In fact, even if you were to patch dummy_hcd.c to instantiate N gadgets,


MODULE_PARM_DESC(num, "number of emulated controllers");

If you set this to 5 you will get 5 pairs of UDC and HCD.


you wouldn't be able to use g_zero.ko, and would be required to rely on
our configfs interface for dynamically generating and binding different
gadget drivers to each of your gadget instances, but that's another
story altogether.


Then you can use configfs and instantiate more then one g_zero 
equivalent and bind it do dummy_udc.0, dummy_udc.1 etc.


Each of them will be connected to a separate bus but they all will be 
visible in the system.


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: [PATCH 2/2] usbip: use monotonic timestamps

2017-11-07 Thread Krzysztof Opasiak




On 11/07/2017 11:39 AM, Arnd Bergmann wrote:

This gets rid of the deprecated do_gettimeofday() function in usbip.
The comment above vgadget_get_frame() mentions that it suffers
from issues with the time jumps due to suspend and settimeofday,
so I'm changing it to use ktime_get_ts64() to use monotonic times
that don't have this problem.

I couldn't tell whether we should use CLOCK_MONOTONIC or
CLOCK_MONOTONIC_RAW here, the difference being the exact rate
when correcting for NTP. I picked monotonic time since it doesn't
change the speed to the existing code and should be better
synchronized with other machines we talk to.

Signed-off-by: Arnd Bergmann 


Looks good to me:
Reviewed-by: Krzysztof Opasiak 

--
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 1/2] usbip: fix off-by-one frame number calculation

2017-11-07 Thread Krzysztof Opasiak




On 11/07/2017 11:39 AM, Arnd Bergmann wrote:

vgadget_get_frame returns a frame number from 0 to 2046, which
may require an expensive division operation to wrap at one lower
than the usual number.

I can't see any reason for this, and all other drivers wrap at
a power-of-two number. My best explanation is that it was a simple
typo, so I'm changing the % modulo operator into a cheaper bitmask
that the other drivers use, to make it wrap after 0x7ff rather than
before it.

Signed-off-by: Arnd Bergmann 


Looks good to me:
Reviewed-by: Krzysztof Opasiak 

--
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 03/11] Creation of "usb_device_auth" LSM hook

2017-06-12 Thread Krzysztof Opasiak


Hi,

On 06/12/2017 06:56 PM, Salvatore Mesoraca wrote:

Creation of a new LSM hook that can be used to authorize or deauthorize
new USB devices via the usb authorization interface.
The same hook can also prevent the authorization of a USB device via
"/sys/bus/usb/devices/DEVICE/authorized".
Using this hook an LSM could provide an higher level of granularity
than the current authorization interface.



Could you please explain me why we need LSM for this?

There are tools like usbguard[1] and as far as I can tell it looks like 
they can do everything for you...

Without kernel modification...
without matching and storing rules inside kernel..
just pure userspace which uses device/interface authorization

Footnote:
1 - https://dkopecek.github.io/usbguard/

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: [PATCH] usb: gadget: f_mass_storage: Fix the logic to iterate all common->luns

2017-06-09 Thread Krzysztof Opasiak




On 06/09/2017 10:18 AM, Axel Lin wrote:

It is wrong to do --i in the for loop.

Fixes: dd02ea5a3305 ("usb: gadget: mass_storage: Use static array for luns")
Signed-off-by: Axel Lin 
---
 drivers/usb/gadget/function/f_mass_storage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
b/drivers/usb/gadget/function/f_mass_storage.c
index 74d57d6..5742813 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2572,7 +2572,7 @@ static int fsg_main_thread(void *common_)
int i;

down_write(&common->filesem);
-   for (i = 0; i < ARRAY_SIZE(common->luns); --i) {
+   for (i = 0; i < ARRAY_SIZE(common->luns); i++) {


OMG... Totally right. I have no idea how I could wrote sth like this.

Reviewed-by: Krzysztof Opasiak 

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: dwc3 udc driver

2017-05-07 Thread Krzysztof Opasiak




On 05/03/2017 03:19 AM, Bin Liu wrote:

On Tue, May 02, 2017 at 08:14:10PM -0500, Bin Liu wrote:

On Tue, May 02, 2017 at 05:05:32PM -0400, Ed Lizard wrote:

I'm having trouble seeing the udc driver for dwc3 in /sys/class/udc.

We are moving platforms from musb on 2.6.37 to dwc3 on 4.6. Whew, a
lot has changed since we were stuck on the old kernel.

It looks like libusbgx and udc is the current method for a complex
video/web gadget ( uvc and a couple of uac gadgets).

Is UDC missing from 4.6?   If so then what kernel should support it?


Yup, dwc3 udc was added since v3.1, I believe.


I meant nope :)



Regards,
-Bin.



I'm going to the latest on linux-next to see if it was not in 4.6 or
it's something I'm doing.


dwc3 udc should present in v4.6. I guess either your kernel wasn't
configured correctly, or your platform (dwc3 glue) wasn't supported in
kernel.



Maybe it's just a naming problem? you are not going to see "dwc3" folder 
in /sys/class/udc like but only device-tree generated name for dwc3 
example for my dwc2 I have 12480000.hsotg.


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: Options for improving f_fs.c performance?

2017-04-21 Thread Krzysztof Opasiak

On 04/21/2017 09:14 AM, Felipe Balbi wrote:

Hi Jerry,

Jerry Zhang writes:

(reposting this question in its own subject, instead of buried in a patchset)

Android has migrated to using f_fs for the mtp function, but in the
process we ran into some performance hiccups in comparison to f_mtp
during bulk transfer of large files.

Allocating memory at boot allowed f_mtp to use large buffers,
around 250kB. Compared to using a 16 or 32 kB buffer, the larger buffer

you don't really need to allocate it at boot time :-) but fair
enough. Also, several (say 200) small buffers (PAGE_SIZE -> 4096 bytes)
would probably be better, but read on.

is faster by almost 50%. f_fs can't use io sizes that large due
to memory fragmentation, since there is a kmalloc on each call.

right

One option here would be to add O_DIRECT as a flag to open
(similar to what is discussed here:
http://www.makelinux.net/ldd3/chp-15-sect-3).

We *could* use O_DIRECT, but I'm not sure that's what's causing
issues. I've been considering how to improve f_fs.c myself for the past
few months, but no time to do it yet.

Then on each io, we could get_user_pages, vmap them together,

get_user_pages_fast() + sg_alloc_table_from_pages() would've been the
best. That way, there would be no copy_to/from_user needed, but that's
something that should happen later. There are lower hanging fruits in
f_fs.c IMO.

and pass that address into ep_queue (for sync io only, non direct io
does not change). This would would allow larger io sizes
to be used and eliminates a user-kernel copy. Having the flag
allows users to decide which endpoints they choose to use this way.

Correct :-)

Does this sound like something that people want or find acceptable?

yes, it _is_ definitely acceptable. But f_fs.c needs a cleanup
first. Also, IMHO a better flag to implement would be O_NONBLOCK. Here's
how f_fs.c works today:

write(ep2, buf, length);
ffs_epfile_write_iter()
ffs_epfile_io()
usb_ep_queue()
wait_for_completion_interruptible()

That wait_for_completion_interruptible() is what's killing
performance. Each and every read/write waits for the USB side to
complete. It would've been much better to have something like:

if (flags & O_NONBLOCK)
wait_for_completion_interruptible()

This would help the write() side of things by a long margin. For reads,
what we could do is have a kernel ring buffer with pre-allocated and
pre-queued usb_requests pointing to different pages in this ring
buffer. When a read() comes, instead of queueing the request right
there, we check if there's data in the internal ring buffer, if there
is, we just copy_to_user(), otherwise we either return 0 or return
-EAGAIN (depending on O_NONBLOCK).

After that, it becomes easy to implement poll() for endpoint files.

Please correct me if I'm wrong but isn't this functionality already
implemented a long time ago using aio and eventfd?

Take a look at example[1] in kernel source.

Footnotes:
1 -
https://github.com/torvalds/linux/blob/master/tools/usb/ffs-aio-example/multibuff/device_app/aio_multibuff.c

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: [PATCH] usb: gadget: remove redundant self assignment

2017-04-19 Thread Krzysztof Opasiak




On 04/17/2017 05:12 AM, Stefan Agner wrote:

The assignment ret = ret is redundant and can be removed.

Signed-off-by: Stefan Agner 
---
A very similar patch has been applied already last year, but there is
a second such assignment...

--
Stefan

 drivers/usb/gadget/udc/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index d685d82dcf48..b57bd53812fe 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -139,10 +139,8 @@ int usb_ep_disable(struct usb_ep *ep)
goto out;

ret = ep->ops->disable(ep);
-   if (ret) {
-   ret = ret;
+   if (ret)
goto out;
-   }

ep->enabled = false;




Reviewed-by: Krzysztof Opasiak 

--
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 1/2] fs: configfs: make qw_sign attribute symmetric

2017-04-19 Thread Krzysztof Opasiak




On 04/15/2017 03:35 AM, Stefan Agner wrote:

Currently qw_sign requires UTF-8 character to set, but returns UTF-16
when read. This isn't obvious when simply using cat since the null
characters are not visible, but hexdump unveils the true string:

  # echo MSFT100 > os_desc/qw_sign
  # hexdump -C os_desc/qw_sign
    4d 00 53 00 46 00 54 00  31 00 30 00 30 00|M.S.F.T.1.0.0.|

Make qw_sign symmetric by returning an UTF-8 string too. Also follow
common convention and add a new line at the end.

Signed-off-by: Stefan Agner 
---
Resend as discussed here:
https://patchwork.kernel.org/patch/9548869/

Sorry, a bit later than we discussed... Hope still not too late?

--
Stefan

 drivers/usb/gadget/configfs.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index cbff3b02840d..863ca4ded1be 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -787,9 +787,13 @@ static ssize_t os_desc_b_vendor_code_store(struct 
config_item *item,
 static ssize_t os_desc_qw_sign_show(struct config_item *item, char *page)
 {
struct gadget_info *gi = os_desc_item_to_gadget_info(item);
+   int res;

-   memcpy(page, gi->qw_sign, OS_STRING_QW_SIGN_LEN);
-   return OS_STRING_QW_SIGN_LEN;
+   res = utf16s_to_utf8s((wchar_t *) gi->qw_sign, OS_STRING_QW_SIGN_LEN,
+ UTF16_LITTLE_ENDIAN, page, PAGE_SIZE - 1);
+   page[res++] = '\n';
+
+   return res;
 }

 static ssize_t os_desc_qw_sign_store(struct config_item *item, const char 
*page,



Code itself looks good to me and from libusbgx perspective it's also 
fine to add this new line as we can just drop it like we do with other 
newlines in case of gadget/config strings.


Reviewed-by: Krzysztof Opasiak 

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: [PATCH 2/2] fs: configfs: use hexadecimal values and new line

2017-04-19 Thread Krzysztof Opasiak




On 04/15/2017 03:35 AM, Stefan Agner wrote:

Other unsigned properties return hexadecimal values, follow this
convention when printing b_vendor_code too. Also add newlines to
the OS Descriptor support related properties, like other sysfs
files use.

Signed-off-by: Stefan Agner 
---
 drivers/usb/gadget/configfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 863ca4ded1be..a22a892de7b7 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -738,7 +738,7 @@ static inline struct gadget_info 
*os_desc_item_to_gadget_info(

 static ssize_t os_desc_use_show(struct config_item *item, char *page)
 {
-   return sprintf(page, "%d",
+   return sprintf(page, "%d\n",
os_desc_item_to_gadget_info(item)->use_os_desc);
 }

@@ -762,7 +762,7 @@ static ssize_t os_desc_use_store(struct config_item *item, 
const char *page,

 static ssize_t os_desc_b_vendor_code_show(struct config_item *item, char *page)
 {
-   return sprintf(page, "%d",
+   return sprintf(page, "0x%02x\n",
os_desc_item_to_gadget_info(item)->b_vendor_code);
 }

@@ -904,7 +904,7 @@ static inline struct usb_os_desc_ext_prop

 static ssize_t ext_prop_type_show(struct config_item *item, char *page)
 {
-   return sprintf(page, "%d", to_usb_os_desc_ext_prop(item)->type);
+   return sprintf(page, "%d\n", to_usb_os_desc_ext_prop(item)->type);
 }

 static ssize_t ext_prop_type_store(struct config_item *item,


looks good to me:

Reviewed-by: Krzysztof Opasiak 
--
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 v3 4/6] usb: usbip tool: Add ncontrollers in vhci_driver structure

2017-04-07 Thread Krzysztof Opasiak




On 04/06/2017 12:03 AM, Yuyang Du wrote:

A new field ncontrollers is added to the vhci_driver structure.
And this field is stored by scanning the vhci_hcd* dirs in the
platform udev.

Suggested-by: Krzysztof Opasiak 
Signed-off-by: Yuyang Du 
---
 tools/usb/usbip/libsrc/vhci_driver.c | 36 
 tools/usb/usbip/libsrc/vhci_driver.h |  1 +
 2 files changed, 37 insertions(+)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index 151580a..f019686 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -7,6 +7,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "sysfs_utils.h"

 #undef  PROGNAME
@@ -134,6 +135,33 @@ static int get_nports(void)
return (int)strtoul(attr_nports, NULL, 10);
 }

+static int vhci_hcd_filter(const struct dirent *dirent)
+{
+   return strcmp(dirent->d_name, "vhci_hcd") >= 0;
+}
+
+static int get_ncontrollers(void)
+{
+   struct dirent **namelist;
+   struct udev_device *platform;
+   int n;
+
+   platform = udev_device_get_parent(vhci_driver->hc_device);
+   if (platform == NULL)
+   return -1;
+
+   n = scandir(udev_device_get_syspath(platform), &namelist, 
vhci_hcd_filter, NULL);
+   if (n < 0)
+   err("scandir failed");
+   else {
+   for (int i = 0; i < n; i++)
+   free(namelist[i]);
+   free(namelist);
+   }
+
+   return n;
+}
+
 /*
  * Read the given port's record.
  *
@@ -231,6 +259,14 @@ int usbip_vhci_driver_open(void)
goto err;
}

+   vhci_driver->ncontrollers = get_ncontrollers();
+   dbg("available controllers: %d", vhci_driver->ncontrollers);
+
+   if (vhci_driver->ncontrollers <=0) {
+   err("no available usb controllers");
+   goto err;
+   }
+
if (refresh_imported_device_list())
goto err;

diff --git a/tools/usb/usbip/libsrc/vhci_driver.h 
b/tools/usb/usbip/libsrc/vhci_driver.h
index fa2316c..33add14 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.h
+++ b/tools/usb/usbip/libsrc/vhci_driver.h
@@ -31,6 +31,7 @@ struct usbip_vhci_driver {
/* /sys/devices/platform/vhci_hcd */
struct udev_device *hc_device;

+   int ncontrollers;
    int nports;
struct usbip_imported_device idev[MAXNPORT];
 };



Reviewed-by: Krzysztof Opasiak 

--
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 v3 3/6] usb: usbip tool: Check the return of get_nports()

2017-04-07 Thread Krzysztof Opasiak




On 04/06/2017 12:03 AM, Yuyang Du wrote:

If we get nonpositive number of ports, there is no sense to
continue, then fail gracefully.

In addition, the commit 0775a9cbc694e8c72 ("usbip: vhci extension:
modifications to vhci driver") introduced configurable numbers of
controllers and ports, but we have a static port number maximum,
MAXNPORT. If exceeded, the idev array will be overflown. We fix
it by validating the nports to make sure the port number max is
not exceeded.

Signed-off-by: Yuyang Du 
---
 tools/usb/usbip/libsrc/vhci_driver.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index f659c14..151580a 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -220,9 +220,17 @@ int usbip_vhci_driver_open(void)
}

vhci_driver->nports = get_nports();
-
dbg("available ports: %d", vhci_driver->nports);

+   if (vhci_driver->nports <=0) {
+   err("no available ports");
+   goto err;
+   }
+   else if (vhci_driver->nports > MAXNPORT) {
+   err("port number exceeds %d", MAXNPORT);
+   goto err;
+   }
+
if (refresh_imported_device_list())
    goto err;




Reviewed-by: Krzysztof Opasiak 

--
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 v2 3/6] usb: usbip tool: Add ncontrollers in vhci_driver structure

2017-04-05 Thread Krzysztof Opasiak




On 04/05/2017 04:03 AM, Yuyang Du wrote:

On Wed, Apr 05, 2017 at 11:46:06AM +0200, Krzysztof Opasiak wrote:



On 04/04/2017 09:08 PM, Yuyang Du wrote:

Hi Krzysztof,

On Tue, Apr 04, 2017 at 04:52:32PM +0200, Krzysztof Opasiak wrote:



On 03/31/2017 02:28 AM, Yuyang Du wrote:

A new field ncontrollers is added to the vhci_driver structure.
And this field is stored by scanning the vhci_hcd* dirs in the
platform udev.

Suggested-by: Krzysztof Opasiak 
Signed-off-by: Yuyang Du 
---
tools/usb/usbip/libsrc/vhci_driver.c | 32 +++-
tools/usb/usbip/libsrc/vhci_driver.h |  1 +
2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index f659c14..ccecd47 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -7,6 +7,7 @@
#include 
#include 
#include 
+#include 
#include "sysfs_utils.h"

#undef  PROGNAME
@@ -134,6 +135,33 @@ static int get_nports(void)
return (int)strtoul(attr_nports, NULL, 10);
}

+static int vhci_hcd_filter(const struct dirent *dirent)
+{
+   return strcmp(dirent->d_name, "vhci_hcd") >= 0 ? 1: 0;


The ? operator may be omitted here as according to doc we need to
return nonzero not 1 exactly.


No, it can't. strcmp() would return negative if not containing "vhci_hcd". E.g.,

strcmp("!@#", "vhci_hcd") ==> -1
strcmp("v", "vhci_hcd") ==> -1


I meant, just to drop the ? itself but leave >= 0

return strcmp(dirent->d_name, "vhci_hcd") >= 0;


Oh, that's doable :)

Send a Reviewed-by ?


I will review also the next version and then give the Reviewed-by as 
there was also some error handling missing in this patch.


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: [PATCH v2 3/6] usb: usbip tool: Add ncontrollers in vhci_driver structure

2017-04-05 Thread Krzysztof Opasiak




On 04/04/2017 09:08 PM, Yuyang Du wrote:

Hi Krzysztof,

On Tue, Apr 04, 2017 at 04:52:32PM +0200, Krzysztof Opasiak wrote:



On 03/31/2017 02:28 AM, Yuyang Du wrote:

A new field ncontrollers is added to the vhci_driver structure.
And this field is stored by scanning the vhci_hcd* dirs in the
platform udev.

Suggested-by: Krzysztof Opasiak 
Signed-off-by: Yuyang Du 
---
tools/usb/usbip/libsrc/vhci_driver.c | 32 +++-
tools/usb/usbip/libsrc/vhci_driver.h |  1 +
2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index f659c14..ccecd47 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -7,6 +7,7 @@
#include 
#include 
#include 
+#include 
#include "sysfs_utils.h"

#undef  PROGNAME
@@ -134,6 +135,33 @@ static int get_nports(void)
return (int)strtoul(attr_nports, NULL, 10);
}

+static int vhci_hcd_filter(const struct dirent *dirent)
+{
+   return strcmp(dirent->d_name, "vhci_hcd") >= 0 ? 1: 0;


The ? operator may be omitted here as according to doc we need to
return nonzero not 1 exactly.


No, it can't. strcmp() would return negative if not containing "vhci_hcd". E.g.,

strcmp("!@#", "vhci_hcd") ==> -1
strcmp("v", "vhci_hcd") ==> -1


I meant, just to drop the ? itself but leave >= 0

return strcmp(dirent->d_name, "vhci_hcd") >= 0;

Cheers,
--
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 v2 6/6] usb: usbip tool: Remove empty lines




On 03/31/2017 02:28 AM, Yuyang Du wrote:

Signed-off-by: Yuyang Du 
---


Greg is probably not going to accept a commit without commit message so 
please fix this.


At least for me it looks like this commit could be squashed with the 
previous one as they are both fixing parse_status()


Cheers,
--
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 v2 5/6] usb: usbip tool: Fix parse_status()




On 03/31/2017 02:28 AM, Yuyang Du wrote:

parse_status() reads the status file one by one, so it can only
update the available and according vhci_driver->idev's.

Signed-off-by: Yuyang Du 
---
 tools/usb/usbip/libsrc/vhci_driver.c | 36 +++-
 tools/usb/usbip/src/usbip_attach.c   |  2 ++
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index 630b139..f596ef4 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -43,11 +43,6 @@ static int parse_status(const char *value)
int ret = 0;
char *c;

-
-   for (int i = 0; i < vhci_driver->nports; i++)
-   memset(&vhci_driver->idev[i], 0, sizeof(vhci_driver->idev[i]));
-
-
/* skip a header line */
c = strchr(value, '\n');
if (!c)
@@ -58,6 +53,7 @@ static int parse_status(const char *value)
int port, status, speed, devid;
unsigned long socket;
char lbusid[SYSFS_BUS_ID_SIZE];
+   struct usbip_imported_device *idev;

ret = sscanf(c, "%d %d %d %x %lx %31s\n",
&port, &status, &speed,
@@ -72,30 +68,28 @@ static int parse_status(const char *value)
port, status, speed, devid);
dbg("socket %lx lbusid %s", socket, lbusid);

-
/* if a device is connected, look at it */
-   {
-   struct usbip_imported_device *idev = 
&vhci_driver->idev[port];
+   idev = &vhci_driver->idev[port];
+
+   memset(idev, 0, sizeof(*idev));

-   idev->port   = port;
-   idev->status = status;
+   idev->port   = port;
+   idev->status = status;

-   idev->devid  = devid;
+   idev->devid  = devid;

-   idev->busnum = (devid >> 16);
-   idev->devnum = (devid & 0x);
+   idev->busnum = (devid >> 16);
+   idev->devnum = (devid & 0x);

-   if (idev->status != VDEV_ST_NULL
-   && idev->status != VDEV_ST_NOTASSIGNED) {
-   idev = imported_device_init(idev, lbusid);
-   if (!idev) {
-   dbg("imported_device_init failed");
-   return -1;
-   }
+   if (idev->status != VDEV_ST_NULL
+   && idev->status != VDEV_ST_NOTASSIGNED) {
+   idev = imported_device_init(idev, lbusid);
+   if (!idev) {
+   dbg("imported_device_init failed");
+   return -1;
}
}

-
/* go to the next line */
c = strchr(c, '\n');
if (!c)
diff --git a/tools/usb/usbip/src/usbip_attach.c 
b/tools/usb/usbip/src/usbip_attach.c
index 70a6b50..62a297f 100644
--- a/tools/usb/usbip/src/usbip_attach.c
+++ b/tools/usb/usbip/src/usbip_attach.c
@@ -108,6 +108,8 @@ static int import_device(int sockfd, struct 
usbip_usb_device *udev)
    return -1;
    }

+   dbg("got free port %d", port);
+
rc = usbip_vhci_attach_device(port, sockfd, udev->busnum,


Apart from this unrelated change above:

Reviewed-by: Krzysztof Opasiak 

Cheers,
--
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 v2 4/6] usb: usbip tool: Fix refresh_imported_device_list()




On 03/31/2017 02:28 AM, Yuyang Du wrote:

The commit 0775a9cbc694e8c7 ("usbip: vhci extension: modifications
to vhci driver") introduced multiple controllers, but the status
of the ports are only extracted from the first status file, fix it.

Signed-off-by: Yuyang Du 
---
 tools/usb/usbip/libsrc/vhci_driver.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index ccecd47..630b139 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -108,18 +108,33 @@ static int parse_status(const char *value)
return 0;
 }

+#define MAX_STATUS_NAME 16
+
 static int refresh_imported_device_list(void)
 {
const char *attr_status;
+   char status[MAX_STATUS_NAME+1] = "status";
+   int i, ret;

-   attr_status = udev_device_get_sysattr_value(vhci_driver->hc_device,
-  "status");
-   if (!attr_status) {
-   err("udev_device_get_sysattr_value failed");
-   return -1;
+   for (i = 0; i < vhci_driver->ncontrollers; i++) {
+   if (i > 0)
+   snprintf(status, sizeof(status), "status.%d", i);
+
+   attr_status = 
udev_device_get_sysattr_value(vhci_driver->hc_device,
+   status);
+   if (!attr_status) {
+   err("udev_device_get_sysattr_value failed");
+   return -1;
+   }
+
+   dbg("controller %d", i);
+
+   ret = parse_status(attr_status);
+   if (ret != 0)
+   return ret;
}

-   return parse_status(attr_status);
+   return 0;
 }


I decided to not urge here with OO programming introduction so just 
ignore my previous comment and add:


Reviewed-by: Krzysztof Opasiak 

Cheers,
--
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 v2 3/6] usb: usbip tool: Add ncontrollers in vhci_driver structure




On 03/31/2017 02:28 AM, Yuyang Du wrote:

A new field ncontrollers is added to the vhci_driver structure.
And this field is stored by scanning the vhci_hcd* dirs in the
platform udev.

Suggested-by: Krzysztof Opasiak 
Signed-off-by: Yuyang Du 
---
 tools/usb/usbip/libsrc/vhci_driver.c | 32 +++-
 tools/usb/usbip/libsrc/vhci_driver.h |  1 +
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index f659c14..ccecd47 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -7,6 +7,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "sysfs_utils.h"

 #undef  PROGNAME
@@ -134,6 +135,33 @@ static int get_nports(void)
return (int)strtoul(attr_nports, NULL, 10);
 }

+static int vhci_hcd_filter(const struct dirent *dirent)
+{
+   return strcmp(dirent->d_name, "vhci_hcd") >= 0 ? 1: 0;


The ? operator may be omitted here as according to doc we need to return 
nonzero not 1 exactly.



+}
+
+static int get_ncontrollers(void)
+{
+   struct dirent **namelist;
+   struct udev_device *platform;
+   int n;
+
+   platform = udev_device_get_parent(vhci_driver->hc_device);
+   if (platform == NULL)
+   return -1;
+
+   n = scandir(udev_device_get_syspath(platform), &namelist, 
vhci_hcd_filter, NULL);
+   if (n < 0)
+   err("scandir failed");
+   else {
+   for (int i = 0; i < n; i++)
+   free(namelist[i]);
+   free(namelist);
+   }
+
+   return n;
+}
+
 /*
  * Read the given port's record.
  *
@@ -220,9 +248,11 @@ int usbip_vhci_driver_open(void)
}

vhci_driver->nports = get_nports();
-


Seems to be unrelated.


dbg("available ports: %d", vhci_driver->nports);

+   vhci_driver->ncontrollers = get_ncontrollers();


shouldn't we check here if we didn't got error from scandir()?

Cheers,
--
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 v2 1/6] usb: usbip: Remove unnecessary get_vdev()




On 03/31/2017 02:28 AM, Yuyang Du wrote:

vhci_tx_urb() should be able to get the vhci_device from
its caller vhci_urb_enqueue(), instead of brutal-force
searching it.

Signed-off-by: Yuyang Du 
---
 drivers/usb/usbip/vhci_hcd.c | 32 ++--
 1 file changed, 2 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index e4cb9f0..5d8b2c2 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -430,36 +430,8 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
return retval;
 }

-static struct vhci_device *get_vdev(struct usb_device *udev)
+static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev)
 {
-   struct platform_device *pdev;
-   struct usb_hcd *hcd;
-   struct vhci_hcd *vhci;
-   int pdev_nr, rhport;
-
-   if (!udev)
-   return NULL;
-
-   for (pdev_nr = 0; pdev_nr < vhci_num_controllers; pdev_nr++) {
-   pdev = *(vhci_pdevs + pdev_nr);
-   if (pdev == NULL)
-   continue;
-   hcd = platform_get_drvdata(pdev);
-   if (hcd == NULL)
-   continue;
-   vhci = hcd_to_vhci(hcd);
-   for (rhport = 0; rhport < VHCI_HC_PORTS; rhport++) {
-   if (vhci->vdev[rhport].udev == udev)
-   return &vhci->vdev[rhport];
-   }
-   }
-
-   return NULL;
-}
-
-static void vhci_tx_urb(struct urb *urb)
-{
-   struct vhci_device *vdev = get_vdev(urb->dev);
struct vhci_priv *priv;
struct vhci_hcd *vhci;
unsigned long flags;
@@ -601,7 +573,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb 
*urb,
}

 out:
-   vhci_tx_urb(urb);
+   vhci_tx_urb(urb, vdev);
spin_unlock_irqrestore(&vhci->lock, flags);

return 0;



I think that I've already gave you my reviewed by for this patch.
Nevertheless:

Reviewed-by: Krzysztof Opasiak 

--
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 5/6] usb: usbip tool: Fix refresh_imported_device_list()




On 03/27/2017 07:25 AM, Yuyang Du wrote:

On Mon, Mar 27, 2017 at 09:07:50AM +0200, Krzysztof Opasiak wrote:


As now we have multiple controllers I would be more than happy if we
could fix functions like this one to take a controller as a
parameter and invoke commands on it instead hardcoding loops like
this one with some unclear conditions like if (i > 0).


You mean something like this?

int parse_controller_status(int controller) {

if (controller > 0)
...

...

}



Rather:

int parse_status(controller *ctrl)
{
stat = get_status(ctrl->status_path);

...
}

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: [PATCH 6/6] usb: usbip tool: Fix parse_status()


+ Shuah Khan

On 03/23/2017 03:46 AM, Yuyang Du wrote:

parse_status() reads the status file one by one, so it can only
update the available and according vhci_driver->idev's.

Signed-off-by: Yuyang Du 
---
 tools/usb/usbip/libsrc/vhci_driver.c | 34 ++
 tools/usb/usbip/src/usbip_attach.c   |  2 ++
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index 1c0e7f0..5b14235 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -40,10 +40,6 @@ static int parse_status(const char *value)
int ret = 0;
char *c;

-   for (int i = 0; i < vhci_driver->nports; i++)
-   memset(&vhci_driver->idev[i], 0, sizeof(vhci_driver->idev[i]));
-
-
/* skip a header line */
c = strchr(value, '\n');
if (!c)
@@ -68,30 +64,28 @@ static int parse_status(const char *value)
port, status, speed, devid);
dbg("socket %lx lbusid %s", socket, lbusid);

-
/* if a device is connected, look at it */
-   {
-   struct usbip_imported_device *idev = 
&vhci_driver->idev[port];
+   struct usbip_imported_device *idev = &vhci_driver->idev[port];

-   idev->port   = port;
-   idev->status = status;
+   memset(&vhci_driver->idev[port], 0, sizeof(struct 
usbip_imported_device));


maybe just:
    memset(idev, 0, sizeof(*idev));


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: [PATCH 5/6] usb: usbip tool: Fix refresh_imported_device_list()



+ Shuah Khan

On 03/23/2017 03:46 AM, Yuyang Du wrote:

The commit 0775a9cbc694e8c7 ("usbip: vhci extension: modifications
to vhci driver") introduced multiple controllers, but the status
of the ports are only extracted from the first status file, fix it.

Signed-off-by: Yuyang Du 
---
 tools/usb/usbip/libsrc/vhci_driver.c | 30 +-
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index d335f04..1c0e7f0 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -35,14 +35,11 @@
return NULL;
 }

-
-


Unrelated (put to separate commit)


 static int parse_status(const char *value)
 {
int ret = 0;
char *c;

-


The same here.


for (int i = 0; i < vhci_driver->nports; i++)
memset(&vhci_driver->idev[i], 0, sizeof(vhci_driver->idev[i]));

@@ -107,18 +104,33 @@ static int parse_status(const char *value)
return 0;
 }

+#define MAX_STATUS_NAME 16
+
 static int refresh_imported_device_list(void)
 {
const char *attr_status;
+   char status[MAX_STATUS_NAME+1] = "status";
+   int i, ret;

-   attr_status = udev_device_get_sysattr_value(vhci_driver->hc_device,
-  "status");
-   if (!attr_status) {
-   err("udev_device_get_sysattr_value failed");
-   return -1;
+   for (i = 0; i < vhci_driver->ncontrollers; i++) {
+   if (i > 0)
+   snprintf(status, MAX_STATUS_NAME+1, "status.%d", i);


s/MAX_STATUS_NAME+1/sizeof(status)


+
+   attr_status = 
udev_device_get_sysattr_value(vhci_driver->hc_device,
+   status);
+   if (!attr_status) {
+   err("udev_device_get_sysattr_value failed");
+   return -1;
+   }
+
+   dbg("controller %d", i);
+
+   ret = parse_status(attr_status);
+   if (ret != 0)
+   return ret;
}

-   return parse_status(attr_status);
+   return 0;
 }


As now we have multiple controllers I would be more than happy if we 
could fix functions like this one to take a controller as a parameter 
and invoke commands on it instead hardcoding loops like this one with 
some unclear conditions like if (i > 0).


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: [PATCH 4/6] usb: usbip tool: Add ncontrollers in vhci_driver structure


+ Shuah Khan

On 03/23/2017 03:46 AM, Yuyang Du wrote:

This field is read from the newly added ncontroller sysfs.

Signed-off-by: Yuyang Du 
---
 tools/usb/usbip/libsrc/vhci_driver.c | 18 +-
 tools/usb/usbip/libsrc/vhci_driver.h |  1 +
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index 4228b98..d335f04 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -134,6 +134,20 @@ static int get_nports(void)
return strtoul(attr_nports, NULL, 10);
 }

+static int get_ncontrollers(void)
+{
+   const char *attr_ncontrollers;
+
+   attr_ncontrollers = 
udev_device_get_sysattr_value(vhci_driver->hc_device,
+ "ncontrollers");
+   if (!attr_ncontrollers) {
+   err("udev_device_get_sysattr_value ncontrollers failed");
+   return -1;
+   }
+
+   return strtoul(attr_ncontrollers, NULL, 10);
+}
+


Why not just scandir()?

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: [PATCH 3/6] usb: usbip: Export the number of VHCI controllers in a sysfs file


+ Shuah Khan

On 03/23/2017 03:46 AM, Yuyang Du wrote:

This number facilitates user space tool to easily know what we have.

Signed-off-by: Yuyang Du 
---
 drivers/usb/usbip/vhci_sysfs.c | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index b96e5b1..af75ffb 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -146,6 +146,16 @@ static ssize_t nports_show(struct device *dev, struct 
device_attribute *attr,
 }
 static DEVICE_ATTR_RO(nports);

+static ssize_t ncontrollers_show(struct device *dev, struct device_attribute 
*attr,
+  char *out)
+{
+   char *s = out;
+
+   out += sprintf(out, "%d\n", vhci_num_controllers);
+   return out - s;
+}
+static DEVICE_ATTR_RO(ncontrollers);
+


Why you need a special RO attribute for this? Can't just cannot check 
content of directory using for example scandir() to learn what we have?


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: [PATCH 2/6] usb: usbip tool: Fix get_nports()

2017-03-26 Thread Krzysztof Opasiak


+ Shuah Khan

On 03/23/2017 03:46 AM, Yuyang Du wrote:

The commit 0775a9cbc694e8c72 ("usbip: vhci extension: modifications
to vhci driver") introduced multiple controllers, and nports as a sys
file, and claimed to read the nports from it, but it didn't.

In addition, the get_nports() has been so wrong that even with 8 port
lines for instance, it gets 7 (I am guessing it is due to a '\n' mess).

Signed-off-by: Yuyang Du 
---
 tools/usb/usbip/libsrc/vhci_driver.c | 28 +---
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index ad92047..4228b98 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -123,33 +123,15 @@ static int refresh_imported_device_list(void)

 static int get_nports(void)
 {
-   char *c;
-   int nports = 0;
-   const char *attr_status;
+   const char *attr_nports;

-   attr_status = udev_device_get_sysattr_value(vhci_driver->hc_device,
-  "status");
-   if (!attr_status) {
-   err("udev_device_get_sysattr_value failed");
+   attr_nports = udev_device_get_sysattr_value(vhci_driver->hc_device, 
"nports");
+   if (!attr_nports) {
+   err("udev_device_get_sysattr_value nports failed");
return -1;
}

-   /* skip a header line */
-   c = strchr(attr_status, '\n');
-   if (!c)
-   return 0;
-   c++;
-
-   while (*c != '\0') {
-   /* go to the next line */
-   c = strchr(c, '\n');
-   if (!c)
-   return nports;
-   c++;
-   nports += 1;
-   }
-
-   return nports;
+   return strtoul(attr_nports, NULL, 10);
 }


As strtoul() returns long and get_nports() should return int I'd prefer 
to do here some explicit cast to int to avoid some potential compiler 
warnings.


After fixing this you may add my:

Reviewed-by: Krzysztof Opasiak 

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: [PATCH 1/6] usb: usbip: Remove unnecessary get_vdev()

2017-03-26 Thread Krzysztof Opasiak


+ Shuah Khan

On 03/27/2017 08:41 AM, Krzysztof Opasiak wrote:



On 03/23/2017 03:46 AM, Yuyang Du wrote:

vhci_tx_urb() should be able to get the vhci_device from
its caller vhci_urb_enqueue(), instead of brutal-force
searching it.

Signed-off-by: Yuyang Du 


Reviewed-by: Krzysztof Opasiak 



--
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 1/6] usb: usbip: Remove unnecessary get_vdev()

2017-03-26 Thread Krzysztof Opasiak




On 03/23/2017 03:46 AM, Yuyang Du wrote:

vhci_tx_urb() should be able to get the vhci_device from
its caller vhci_urb_enqueue(), instead of brutal-force
searching it.

Signed-off-by: Yuyang Du 


Reviewed-by: Krzysztof Opasiak 

--
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: Question: Does usbip support USB 3.0?

2017-03-08 Thread Krzysztof Opasiak




On 03/08/2017 12:36 AM, Yuyang Du wrote:

Hi Krzysztof,

On Tue, Mar 07, 2017 at 10:25:31AM +0100, Krzysztof Opasiak wrote:


If it' not some "top secret" device maybe try to send us descriptors
(lsusb -vd VID:PID)?


Pasted at the end. Surprisingly, I don't see isochronous types. Is it
bulk streams?


Descriptors says that it's bulk so now question is if it uses streams or 
not. Check your driver or USB traffic.


Cheers,
--
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: Question: Does usbip support USB 3.0?

2017-03-07 Thread Krzysztof Opasiak




On 03/07/2017 01:52 AM, Yuyang Du wrote:

Hi Greg,

On Mon, Mar 06, 2017 at 11:05:47AM +0100, Greg KH wrote:

I did the experiment. Our device "requires" that the SUPER_SPPED be used.
So, we are attempting to add SUPER_SPEED support to usbip. To assess the
effort, could you please give us some pointers on how to do it? And what
are the difficulties?


As others have pointed out, you will have to change the code to do this,
and it will be easier if you only need a speed change, and not streams.
But why do you really need a speed indicator?  What happens if you just
report that the device is "high speed" to the host instead?  It should
not affect the transmit speeds, right?


I don't understand how to just "report" the device as "high speed". Doesn't
the device specify which speed? Moreover, it seems the device doesn't compromise
on speed - when I inserted it into a USB 2.0 port, it doesn't work.


Does dmesg says sth why new device cannot be enumerated?

If it' not some "top secret" device maybe try to send us descriptors 
(lsusb -vd VID:PID)?


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: Question: Does usbip support USB 3.0?

2017-03-07 Thread Krzysztof Opasiak


Hi,

On 03/07/2017 01:43 AM, Yuyang Du wrote:

( ... )


There is also other side, the protocol. Probably we won't be able to
handle this using exiting protocol messages. At first glance I see
at least one reason:

Documentation/usb/usbip_protocol.txt:

USBIP_RET_SUBMIT: Reply for submitting an URB

 Offset| Length | Value  | Description
---+++---
 0 | 4  | 0x0003 | command
---+++---
 4 | 4  || seqnum: URB sequence number
---+++---
 8 | 4  || devid
---+++---
 0xC   | 4  || direction: 0: USBIP_DIR_OUT
   |||1: USBIP_DIR_IN
---+++---
 0x10  | 4  || ep: endpoint number
---+++---
 0x14  | 4  || status: zero for successful URB
transaction,
   |||   otherwise some kind of error happened.
---+++---
 0x18  | 4  | n  | actual_length: number of URB data bytes
---+++---
 0x1C  | 4  || start_frame: for an ISO frame the
actually
   |||   selected frame for transmit.
---+++---
 0x20  | 4  || number_of_packets
---+++---
 0x24  | 4  || error_count
---+++---
 0x28  | 8  || setup: data bytes for USB setup,
filled with
   |||   zeros if not used
---+++---
 0x30  | n  || URB data bytes. For ISO transfers
the padding
   |||   between each ISO packets is not
transmitted.



As you see we have a field for devid, direction and ep but we don't
have a field for stream id.


Then, as Oliver pointed out, if we don't need bulk streams transfer type, the
current protocol should work fine, right?


Yeah it should or at least for now I don't see any obvious reason why it 
could not work.


Cheers,
--
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: Cannot dump USB report descriptors, not even after unbinding the device

2017-03-06 Thread Krzysztof Opasiak



Hi,

On 03/03/2017 09:48 PM, Alan Stern wrote:

On Fri, 3 Mar 2017, László Monda wrote:


Hi Krzysztof, and thank you for the help!


Oh now I see. I missed this when I was reading this email for the first
time.


root@spark ~ # echo -n 2-10.3 > /sys/bus/usb/drivers/usb/unbind



You are trying to unbind here wrong driver I think. Try to unbound driver
for interface instead of for whole device:

# echo -n 2-10.3:1.0 > /sys/bus/usb/drivers/usb/unbind


I've tried to unbind 2-10.3:[0-4].[0-4] but bash always tells me that
there's no such device.


Krzysztof told you the wrong driver.  You should do:

echo 2-10.3:1.0 >/sys/bus/usb/drivers/usbhid/unbind

(the -n doesn't matter).



Thank you Alan for catching this!

I've copy-pasted the path from above and forgot to fix driver name. 
Unfortunately I did the same mistake in my second response. Please 
excuse me Laszlo for this oversight.


Cheers,
--
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: Question: Does usbip support USB 3.0?

2017-03-06 Thread Krzysztof Opasiak



Hi,

On 03/06/2017 02:02 AM, Yuyang Du wrote:

Hi Greg,

On Thu, Feb 23, 2017 at 08:51:03AM +0100, Greg KH wrote:

And again, what specifically are you referring to, and again, have you
tried the code out yourself?  What is preventing you from testing this
in your environment to determine if it works properly for you or not?


I did the experiment. Our device "requires" that the SUPER_SPPED be used.
So, we are attempting to add SUPER_SPEED support to usbip. To assess the
effort, could you please give us some pointers on how to do it? And what
are the difficulties?


In the beginning I would start with this commit:

1cd8fd2887e162ad3d067150962cc3d32dcf3150

This commit adds Super Speed support to dummy_hcd so from the kernel 
point of view this should be a good source of knowledge how to add Super 
Speed mode to existing HCD.


There is also other side, the protocol. Probably we won't be able to 
handle this using exiting protocol messages. At first glance I see at 
least one reason:


Documentation/usb/usbip_protocol.txt:

USBIP_RET_SUBMIT: Reply for submitting an URB

 Offset| Length | Value  | Description
---+++---
 0 | 4  | 0x0003 | command
---+++---
 4 | 4  || seqnum: URB sequence number
---+++---
 8 | 4  || devid
---+++---
 0xC   | 4  || direction: 0: USBIP_DIR_OUT
   |||1: USBIP_DIR_IN
---+++---
 0x10  | 4  || ep: endpoint number
---+++---
 0x14  | 4  || status: zero for successful URB 
transaction,

   |||   otherwise some kind of error happened.
---+++---
 0x18  | 4  | n  | actual_length: number of URB data bytes
---+++---
 0x1C  | 4  || start_frame: for an ISO frame the 
actually

   |||   selected frame for transmit.
---+++---
 0x20  | 4  || number_of_packets
---+++---
 0x24  | 4  || error_count
---+++---
 0x28  | 8  || setup: data bytes for USB setup, 
filled with

   |||   zeros if not used
---+++---
 0x30  | n  || URB data bytes. For ISO transfers 
the padding
   |||   between each ISO packets is not 
transmitted.




As you see we have a field for devid, direction and ep but we don't have 
a field for stream id.


Cheers,
--
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: Cannot dump USB report descriptors, not even after unbinding the device

2017-03-03 Thread Krzysztof Opasiak

Hi,

>Hi Krzysztof, and thank you for the help!
>
>> Oh now I see. I missed this when I was reading this email for the first
>> time.
>>
>> root@spark ~ # echo -n 2-10.3 > /sys/bus/usb/drivers/usb/unbind
>>
>>
>> You are trying to unbind here wrong driver I think. Try to unbound driver
>> for interface instead of for whole device:
>>
>> # echo -n 2-10.3:1.0 > /sys/bus/usb/drivers/usb/unbind
>
> I've tried to unbind 2-10.3:[0-4].[0-4] but bash always tells me that
> there's no such device.
>
> The strace output is very long, and I can't make much sense of it, but
> I uploaded it as a GitHub gist:
> https://gist.github.com/mondalaci/424d145263b5b6160d02bd42ce80247f

This output is quite similar to what I expected. This part is important:

ioctl(10, USBDEVFS_CLAIMINTERFACE, 0x7fff881fc78c) = -1 ENOENT (No such
file or directory)
write(1, " Report Descriptors: \n", 30) = 30
write(1, " ** UNAVAILABLE **\n", 29) = 29

So lsusb is trying to claim your hid interface but it gets the ENOENT
error and that's nothing weird because before executing lsusb you
kicked-off the generic USB device driver:

echo -n 2-10.3 > /sys/bus/usb/drivers/usb/unbind

which is responsible for setting USB device configuration and when you
unbind it from your device it just simply puts whole device in
unconfigured state. This means that you cannot claim any interface
because they simply don't exist in your system device hierarchy.

So to make things simply working I suggest to:
1) Disconnect your device (or alternatively just rebind the driver;))

2) Go to /sys/bus/usb/devices

3) Find directory which corresponds to your device (for example 2-10.3)

4) As you have 3 interfaces you will then have 3 directories which names
starts with name of your device but then there is additional : and two
digits. (for example 2-10.3:1.0). The last digit is bInterfaceNumber.

5) Use lsusb -t to check to which interfaces your driver is bound.

6) Unbind first driver using:
# echo -n 2-10.3:1.X > /sys/bus/usb/drivers/usb/unbind

Where X is bInterfaceNumber of one of interfaces in your device to which
some driver is bound.

7) Execute lsusb -t once again to check if there are some drivers left.
If yes goto 6.

8) Execute your lsusb command to get your report descriptors.

Cheers,
Krzysztof Opasiak
--
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: Cannot dump USB report descriptors, not even after unbinding the device

2017-03-03 Thread Krzysztof Opasiak




On 03/03/2017 04:59 PM, László Monda wrote:

Dear List,

About 2 years ago, I used to be able to dump the USB report
descriptors of my input devices after unlinking them, but for some
reason, not anymore.

root@spark ~ # dmesg | tail
[239486.804224] logitech-hidpp-device 0003:046D:404D.02B3:
input,hidraw5: USB HID v1.11 Keyboard [Logitech K400 Plus] on
usb-:00:14.0-10.3:1
[239535.214726] usb 2-10.3: USB disconnect, device number 115
[239538.010455] usb 2-10.3: new full-speed USB device number 116 using xhci_hcd
[239538.125768] usb 2-10.3: New USB device found, idVendor=046d, idProduct=c52b
[239538.125770] usb 2-10.3: New USB device strings: Mfr=1, Product=2,
SerialNumber=0
[239538.125771] usb 2-10.3: Product: USB Receiver
[239538.125772] usb 2-10.3: Manufacturer: Logitech
[239538.135925] logitech-djreceiver 0003:046D:C52B.02B6:
hiddev0,hidraw4: USB HID v1.11 Device [Logitech USB Receiver] on
usb-:00:14.0-10.3/input2
[239538.264789] input: Logitech K400 Plus as
/devices/pci:00/:00:14.0/usb2/2-10/2-10.3/2-10.3:1.2/0003:046D:C52B.02B6/0003:046D:404D.02B7/input/input340
[239538.264987] logitech-hidpp-device 0003:046D:404D.02B7:
input,hidraw5: USB HID v1.11 Keyboard [Logitech K400 Plus] on
usb-:00:14.0-10.3:1



Oh now I see. I missed this when I was reading this email for the first 
time.



root@spark ~ # echo -n 2-10.3 > /sys/bus/usb/drivers/usb/unbind


You are trying to unbind here wrong driver I think. Try to unbound 
driver for interface instead of for whole device:


# echo -n 2-10.3:1.0 > /sys/bus/usb/drivers/usb/unbind

Cheers,
--
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: Cannot dump USB report descriptors, not even after unbinding the device

2017-03-03 Thread Krzysztof Opasiak




On 03/03/2017 04:59 PM, László Monda wrote:

Dear List,

About 2 years ago, I used to be able to dump the USB report
descriptors of my input devices after unlinking them, but for some
reason, not anymore.

root@spark ~ # dmesg | tail
[239486.804224] logitech-hidpp-device 0003:046D:404D.02B3:
input,hidraw5: USB HID v1.11 Keyboard [Logitech K400 Plus] on
usb-:00:14.0-10.3:1
[239535.214726] usb 2-10.3: USB disconnect, device number 115
[239538.010455] usb 2-10.3: new full-speed USB device number 116 using xhci_hcd
[239538.125768] usb 2-10.3: New USB device found, idVendor=046d, idProduct=c52b
[239538.125770] usb 2-10.3: New USB device strings: Mfr=1, Product=2,
SerialNumber=0
[239538.125771] usb 2-10.3: Product: USB Receiver
[239538.125772] usb 2-10.3: Manufacturer: Logitech
[239538.135925] logitech-djreceiver 0003:046D:C52B.02B6:
hiddev0,hidraw4: USB HID v1.11 Device [Logitech USB Receiver] on
usb-:00:14.0-10.3/input2
[239538.264789] input: Logitech K400 Plus as
/devices/pci:00/:00:14.0/usb2/2-10/2-10.3/2-10.3:1.2/0003:046D:C52B.02B6/0003:046D:404D.02B7/input/input340
[239538.264987] logitech-hidpp-device 0003:046D:404D.02B7:
input,hidraw5: USB HID v1.11 Keyboard [Logitech K400 Plus] on
usb-:00:14.0-10.3:1

root@spark ~ # echo -n 2-10.3 > /sys/bus/usb/drivers/usb/unbind

root@spark ~ # lsusb -vd 046d:c52b


Try to use strace and see what fails:

strace lsusb -vd 046d:c52b

Cheers,
--
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] usb: gadget: add RNDIS configfs option for Windows rndiscmp.inf compatibility

2017-03-01 Thread Krzysztof Opasiak




On 02/28/2017 10:58 PM, David Lechner wrote:

This adds a new configfs attribute named `use_ms_rndiscmp`. It is a
boolean value that is used to select the class/subclass/protocol used
by the RNDIS function interface association descriptor. By default,
this is 0x02 (Comm), 0x06 (Ethernet), 0xff (None). When the
use_ms_rndiscmp attribute is set to true, the values 0xef (Misc),
0x04 (RNDIS), 0x01 (Ethernet) will be used instead. This class/subclass/
protocol combination is recognized by the rndiscmp.inf file in Windows
Vista and newer and will cause Windows to load the correct RNDIS driver
without the need for a custom (signed) .inf file.



To be honest, I'm not very happy with this patch because it makes our 
ConfigFS interface inflexible.


Let's assume that any other combination of this attributes will be 
needed in a future and then what we are going to do with use_ms_rndiscmp 
attribute?


So instead of having single attribute which sets the whole triple of 
values to some hardcoded ones I would prefer to have one attribute per 
each of this values and allow user to set them to his own values from 
userspace.


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: Question: Does usbip support USB 3.0?

2017-02-28 Thread Krzysztof Opasiak




On 02/23/2017 08:51 AM, Greg KH wrote:

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Thu, Feb 23, 2017 at 07:43:23AM +, Du, Yuyang wrote:

Oh, then let me try to simplify it, what if it's an only USB 3.0 device?


I have a USB keyboard around here somewhere that is a "USB 3.0" device,
that is running at the USB "low speed" rate. :)

Again, USB 3.0 supports all speed devices, see the spec for all of the
details.



Just to add what Greg said:

vhci (virtual host controller for usbip) itself is high speed and does 
not support stuff like streams etc. which are included in USB 3.0 spec 
so you won't be able to connect USB device in super speed mode via usbip.


So if your device supports high speed mode it will work fine. If your 
device doesn't work with speed lower than super speed then you won't be 
able to use it via usbip.


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: [PATCH v2 1/2] usbip: Fix-format-overflow

2017-02-21 Thread Krzysztof Opasiak


Hi,

W dniu 2017-02-21 o 18:57, Jonathan Dieter pisze:

The usbip userspace tools call sprintf()/snprintf() and don't check for
the return value which can lead the paths to overflow, truncating the
final file in the path.

More urgently, GCC 7 now warns that these aren't checked with
-Wformat-overflow, and with -Werror enabled in configure.ac, that makes
these tools unbuildable.

This patch fixes these problems by replacing sprintf() with snprintf() in
one place and adding checks for the return value of snprintf().

Reviewed-by: Peter Senna Tschudin 
Signed-off-by: Jonathan Dieter 
---
 tools/usb/usbip/libsrc/usbip_common.c  |  8 +++-
 tools/usb/usbip/libsrc/usbip_host_common.c | 25 -
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/tools/usb/usbip/libsrc/usbip_common.c 
b/tools/usb/usbip/libsrc/usbip_common.c
index ac73710..01dd4b2 100644
--- a/tools/usb/usbip/libsrc/usbip_common.c
+++ b/tools/usb/usbip/libsrc/usbip_common.c
@@ -215,9 +215,15 @@ int read_usb_interface(struct usbip_usb_device *udev, int 
i,
   struct usbip_usb_interface *uinf)
 {
char busid[SYSFS_BUS_ID_SIZE];
+   unsigned int size;


I'm not really convinced to use unsigned here. snprintf() is declared to 
return signed integer so we should assume that some of its 
implementation may return negative error code. Any rationale to this 
instead of just doing a cast for comparsion but signed value to print error?


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: [PATCH 1/2] Fix format overflows

2017-02-20 Thread Krzysztof Opasiak



Hi,

W dniu 2017-02-20 o 21:51, Jonathan Dieter pisze:

The usbip userspace tools call sprintf()/snprintf() and don't check for
the return value which can lead the paths to overflow, truncating the
final file in the path.

More urgently, GCC 7 now warns that these aren't checked with
-Wformat-overflow, and with -Werror enabled in configure.ac, that makes
these tools unbuildable.

This patch fixes these problems by replacing sprintf() with snprintf() in
one place and adding checks for the return value of snprintf().

Reviewed-by: Peter Senna Tschudin 
Signed-off-by: Jonathan Dieter 
---
 tools/usb/usbip/libsrc/usbip_common.c  |  8 +++-
 tools/usb/usbip/libsrc/usbip_host_common.c | 25 -
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/tools/usb/usbip/libsrc/usbip_common.c 
b/tools/usb/usbip/libsrc/usbip_common.c
index ac73710..fc875ee 100644
--- a/tools/usb/usbip/libsrc/usbip_common.c
+++ b/tools/usb/usbip/libsrc/usbip_common.c
@@ -215,9 +215,15 @@ int read_usb_interface(struct usbip_usb_device *udev, int 
i,
   struct usbip_usb_interface *uinf)
 {
char busid[SYSFS_BUS_ID_SIZE];
+   int size;
struct udev_device *sif;

-   sprintf(busid, "%s:%d.%d", udev->busid, udev->bConfigurationValue, i);
+   size = snprintf(busid, SYSFS_BUS_ID_SIZE, "%s:%d.%d",


why not sizeof(busid)?


+   udev->busid, udev->bConfigurationValue, i);
+   if (size >= SYSFS_BUS_ID_SIZE) {


As above.


+   err("busid length %i >= SYSFS_BUS_ID_SIZE", size);
+   return -1;
+   }

sif = udev_device_new_from_subsystem_sysname(udev_context, "usb", 
busid);
if (!sif) {
diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c 
b/tools/usb/usbip/libsrc/usbip_host_common.c
index 9d41522..690cd49 100644
--- a/tools/usb/usbip/libsrc/usbip_host_common.c
+++ b/tools/usb/usbip/libsrc/usbip_host_common.c
@@ -40,13 +40,19 @@ struct udev *udev_context;
 static int32_t read_attr_usbip_status(struct usbip_usb_device *udev)
 {
char status_attr_path[SYSFS_PATH_MAX];
+   int size;
int fd;
int length;
char status;
int value = 0;

-   snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status",
-udev->path);
+   size = snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status",


The same here.


+   udev->path);
+   if (size >= SYSFS_PATH_MAX) {
+   err("usbip_status path length %i >= SYSFS_PATH_MAX", size);
+   return -1;
+   }
+

fd = open(status_attr_path, O_RDONLY);
if (fd < 0) {
@@ -218,6 +224,7 @@ int usbip_export_device(struct usbip_exported_device *edev, 
int sockfd)
 {
char attr_name[] = "usbip_sockfd";
char sockfd_attr_path[SYSFS_PATH_MAX];
+   int size;
char sockfd_buff[30];
int ret;

@@ -237,10 +244,18 @@ int usbip_export_device(struct usbip_exported_device 
*edev, int sockfd)
}

/* only the first interface is true */
-   snprintf(sockfd_attr_path, sizeof(sockfd_attr_path), "%s/%s",
-edev->udev.path, attr_name);
+   size = snprintf(sockfd_attr_path, sizeof(sockfd_attr_path), "%s/%s",
+   edev->udev.path, attr_name);
+   if (size >= SYSFS_PATH_MAX) {


hmmm this should be sizeof(sockfd_attr_path) not SYSFS_PATH_MAX


+   err("exported device path length %i >= SYSFS_PATH_MAX", size);
+   return -1;
+   }

-   snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd);
+   size = snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd);
+   if (size >= 30) {


Please don't hardcode such values in if. use sizeof() like one line above


+   err("socket length %i >= 30", size);
+   return -1;
+   }

ret = write_sysfs_attribute(sockfd_attr_path, sockfd_buff,
strlen(sockfd_buff));



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

[PATCH] usb: gadget: f_hid: fix: Don't access hidg->req without spinlock held

2017-01-31 Thread Krzysztof Opasiak

hidg->req should be accessed only with write_spinlock held as it is
set to NULL when we get disabled by host.

Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/function/f_hid.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c 
b/drivers/usb/gadget/function/f_hid.c
index 89b48bcc377a..5eea44823ca0 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -367,7 +367,7 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
count  = min_t(unsigned, count, hidg->report_length);
 
spin_unlock_irqrestore(&hidg->write_spinlock, flags);
-   status = copy_from_user(hidg->req->buf, buffer, count);
+   status = copy_from_user(req->buf, buffer, count);
 
if (status != 0) {
ERROR(hidg->func.config->cdev,
@@ -378,9 +378,9 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
 
spin_lock_irqsave(&hidg->write_spinlock, flags);
 
-   /* we our function has been disabled by host */
+   /* when our function has been disabled by host */
if (!hidg->req) {
-   free_ep_req(hidg->in_ep, hidg->req);
+   free_ep_req(hidg->in_ep, req);
/*
 * TODO
 * Should we fail with error here?
@@ -394,7 +394,7 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
req->complete = f_hidg_req_complete;
req->context  = hidg;
 
-   status = usb_ep_queue(hidg->in_ep, hidg->req, GFP_ATOMIC);
+   status = usb_ep_queue(hidg->in_ep, req, GFP_ATOMIC);
if (status < 0) {
ERROR(hidg->func.config->cdev,
"usb_ep_queue error on int endpoint %zd\n", status);
-- 
2.9.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 2/2] usb: gadget: function: f_fs: pass companion descriptor along

2017-01-31 Thread Krzysztof Opasiak



On 01/31/2017 04:56 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Krzysztof Opasiak  writes:
>> On 01/31/2017 02:08 PM, Felipe Balbi wrote:
>>> If we're dealing with SuperSpeed endpoints, we need
>>> to make sure to pass along the companion descriptor
>>> and initialize fields needed by the Gadget
>>> API. Eventually, f_fs.c should be converted to use
>>> config_ep_by_speed() like all other functions,
>>> though.
>>>
>>> Cc: 
>>> Signed-off-by: Felipe Balbi 
>>> ---
>>>
>>> Will be sent in a pull request during v4.11-rc
>>>
>>>  drivers/usb/gadget/function/f_fs.c | 15 +--
>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_fs.c 
>>> b/drivers/usb/gadget/function/f_fs.c
>>> index 87fccf611b69..86aba2ebb3ef 100644
>>> --- a/drivers/usb/gadget/function/f_fs.c
>>> +++ b/drivers/usb/gadget/function/f_fs.c
>>> @@ -1833,11 +1833,14 @@ static int ffs_func_eps_enable(struct ffs_function 
>>> *func)
>>> spin_lock_irqsave(&func->ffs->eps_lock, flags);
>>> while(count--) {
>>> struct usb_endpoint_descriptor *ds;
>>> +   struct usb_ss_ep_comp_descriptor *comp_desc = NULL;
>>> +   int needs_comp_desc = false;
>>> int desc_idx;
>>>  
>>> -   if (ffs->gadget->speed == USB_SPEED_SUPER)
>>> +   if (ffs->gadget->speed == USB_SPEED_SUPER) {
>>> desc_idx = 2;
>>> -   else if (ffs->gadget->speed == USB_SPEED_HIGH)
>>> +   needs_comp_desc = true;
>>> +   } else if (ffs->gadget->speed == USB_SPEED_HIGH)
>>> desc_idx = 1;
>>> else
>>> desc_idx = 0;
>>> @@ -1854,6 +1857,14 @@ static int ffs_func_eps_enable(struct ffs_function 
>>> *func)
>>>  
>>> ep->ep->driver_data = ep;
>>> ep->ep->desc = ds;
>>> +
>>> +   comp_desc = (struct usb_ss_ep_comp_descriptor *)(ds +
>>> +   USB_DT_ENDPOINT_SIZE);
>>> +   ep->ep->maxburst = comp_desc->bMaxBurst + 1;
>>> +
>>> +   if (needs_comp_desc)
>>> +   ep->ep->comp_desc = comp_desc;
>>> +
>>
>> Please correct me if I'm wrong but wouldn't we read rubbish here if user
>> provided us SS ep descriptor without companion descriptor?
> 
> companion desc is required for SS endpoints, it's also required that
> they follow EP desc. If user doesn't write it, don't they deserve the
> errors they'll have?
> 

But do we deserve to access potentially unallocated memory inside kernel
each time when some malicious application requests this?;)

In my humble opinion user should get -EINVAL or sth like this from
write(desc, sizeof(desc)) instead of some random data in companion
descriptor.

Cheers,
-- 
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 2/2] usb: gadget: function: f_fs: pass companion descriptor along

2017-01-31 Thread Krzysztof Opasiak



On 01/31/2017 02:08 PM, Felipe Balbi wrote:
> If we're dealing with SuperSpeed endpoints, we need
> to make sure to pass along the companion descriptor
> and initialize fields needed by the Gadget
> API. Eventually, f_fs.c should be converted to use
> config_ep_by_speed() like all other functions,
> though.
> 
> Cc: 
> Signed-off-by: Felipe Balbi 
> ---
> 
> Will be sent in a pull request during v4.11-rc
> 
>  drivers/usb/gadget/function/f_fs.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 87fccf611b69..86aba2ebb3ef 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -1833,11 +1833,14 @@ static int ffs_func_eps_enable(struct ffs_function 
> *func)
>   spin_lock_irqsave(&func->ffs->eps_lock, flags);
>   while(count--) {
>   struct usb_endpoint_descriptor *ds;
> + struct usb_ss_ep_comp_descriptor *comp_desc = NULL;
> + int needs_comp_desc = false;
>   int desc_idx;
>  
> - if (ffs->gadget->speed == USB_SPEED_SUPER)
> + if (ffs->gadget->speed == USB_SPEED_SUPER) {
>   desc_idx = 2;
> - else if (ffs->gadget->speed == USB_SPEED_HIGH)
> + needs_comp_desc = true;
> + } else if (ffs->gadget->speed == USB_SPEED_HIGH)
>   desc_idx = 1;
>   else
>   desc_idx = 0;
> @@ -1854,6 +1857,14 @@ static int ffs_func_eps_enable(struct ffs_function 
> *func)
>  
>   ep->ep->driver_data = ep;
>   ep->ep->desc = ds;
> +
> + comp_desc = (struct usb_ss_ep_comp_descriptor *)(ds +
> + USB_DT_ENDPOINT_SIZE);
> + ep->ep->maxburst = comp_desc->bMaxBurst + 1;
> +
> + if (needs_comp_desc)
> +     ep->ep->comp_desc = comp_desc;
> +

Please correct me if I'm wrong but wouldn't we read rubbish here if user
provided us SS ep descriptor without companion descriptor?


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

[PATCH v2] usb: gadget: f_hid: fix: Move IN request allocation to set_alt()

2017-01-23 Thread Krzysztof Opasiak

Since commit: ba1582f22231 ("usb: gadget: f_hid: use alloc_ep_req()")
we cannot allocate any requests in bind() as we check if we should
align request buffer based on endpoint descriptor which is assigned
in set_alt().

Allocating request in bind() function causes a NULL pointer
dereference.

This commit moves allocation of IN request from bind() to set_alt()
to prevent this issue.

Fixes: ba1582f22231 ("usb: gadget: f_hid: use alloc_ep_req()")
Cc: sta...@vger.kernel.org
Tested-by: David Lechner 
Signed-off-by: Krzysztof Opasiak 
---
Changes since v1:
- rebased on top of testing/next
---
 drivers/usb/gadget/function/f_hid.c | 89 -
 1 file changed, 67 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c 
b/drivers/usb/gadget/function/f_hid.c
index b62e69d..89b48bc 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -338,6 +338,7 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
size_t count, loff_t *offp)
 {
struct f_hidg *hidg  = file->private_data;
+   struct usb_request *req;
unsigned long flags;
ssize_t status = -ENOMEM;
 
@@ -347,7 +348,7 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
spin_lock_irqsave(&hidg->write_spinlock, flags);
 
 #define WRITE_COND (!hidg->write_pending)
-
+try_again:
/* write queue */
while (!WRITE_COND) {
spin_unlock_irqrestore(&hidg->write_spinlock, flags);
@@ -362,6 +363,7 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
}
 
hidg->write_pending = 1;
+   req = hidg->req;
count  = min_t(unsigned, count, hidg->report_length);
 
spin_unlock_irqrestore(&hidg->write_spinlock, flags);
@@ -374,24 +376,38 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
goto release_write_pending;
}
 
-   hidg->req->status   = 0;
-   hidg->req->zero = 0;
-   hidg->req->length   = count;
-   hidg->req->complete = f_hidg_req_complete;
-   hidg->req->context  = hidg;
+   spin_lock_irqsave(&hidg->write_spinlock, flags);
+
+   /* we our function has been disabled by host */
+   if (!hidg->req) {
+   free_ep_req(hidg->in_ep, hidg->req);
+   /*
+* TODO
+* Should we fail with error here?
+*/
+   goto try_again;
+   }
+
+   req->status   = 0;
+   req->zero = 0;
+   req->length   = count;
+   req->complete = f_hidg_req_complete;
+   req->context  = hidg;
 
status = usb_ep_queue(hidg->in_ep, hidg->req, GFP_ATOMIC);
if (status < 0) {
ERROR(hidg->func.config->cdev,
"usb_ep_queue error on int endpoint %zd\n", status);
-   goto release_write_pending;
+   goto release_write_pending_unlocked;
} else {
status = count;
}
+   spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 
return status;
 release_write_pending:
spin_lock_irqsave(&hidg->write_spinlock, flags);
+release_write_pending_unlocked:
hidg->write_pending = 0;
spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 
@@ -595,12 +611,23 @@ static void hidg_disable(struct usb_function *f)
kfree(list);
}
spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+
+   spin_lock_irqsave(&hidg->write_spinlock, flags);
+   if (!hidg->write_pending) {
+   free_ep_req(hidg->in_ep, hidg->req);
+   hidg->write_pending = 1;
+   }
+
+   hidg->req = NULL;
+   spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 }
 
 static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 {
struct usb_composite_dev*cdev = f->config->cdev;
struct f_hidg   *hidg = func_to_hidg(f);
+   struct usb_request  *req_in = NULL;
+   unsigned long   flags;
int i, status = 0;
 
VDBG(cdev, "hidg_set_alt intf:%d alt:%d\n", intf, alt);
@@ -621,6 +648,12 @@ static int hidg_set_alt(struct usb_function *f, unsigned 
intf, unsigned alt)
goto fail;
}
hidg->in_ep->driver_data = hidg;
+
+   req_in = hidg_alloc_ep_req(hidg->in_ep, hidg->report_length);
+   if (!req_in) {
+   status = -ENOMEM;
+   goto disable_ep_in;
+   }
}
 
 
@@ -632,12 +665,12 @@ st

[PATCH 2/4] usb: gadget: f_hid: fix: Prevent accessing released memory

When we unlock our spinlock to copy data to user we may get
disabled by USB host and free the whole list of completed out
requests including the one from which we are copying the data
to user memory.

To prevent from this let's remove our working element from
the list and place it back only if there is sth left when we
finish with it.

Fixes: 99c515005857 ("usb: gadget: hidg: register OUT INT endpoint for 
SET_REPORT")
Cc: sta...@vger.kernel.org
Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/function/f_hid.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c 
b/drivers/usb/gadget/function/f_hid.c
index ceb1d0c..b914e57 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -223,6 +223,13 @@ static ssize_t f_hidg_read(struct file *file, char __user 
*buffer,
/* pick the first one */
list = list_first_entry(&hidg->completed_out_req,
struct f_hidg_req_list, list);
+
+   /*
+* Remove this from list to protect it from beign free()
+* while host disables our function
+*/
+   list_del(&list->list);
+
req = list->req;
count = min_t(unsigned int, count, req->actual - list->pos);
spin_unlock_irqrestore(&hidg->spinlock, flags);
@@ -238,15 +245,20 @@ static ssize_t f_hidg_read(struct file *file, char __user 
*buffer,
 * call, taking into account its current read position.
 */
if (list->pos == req->actual) {
-   spin_lock_irqsave(&hidg->spinlock, flags);
-   list_del(&list->list);
kfree(list);
-   spin_unlock_irqrestore(&hidg->spinlock, flags);
 
req->length = hidg->report_length;
ret = usb_ep_queue(hidg->out_ep, req, GFP_KERNEL);
-   if (ret < 0)
+   if (ret < 0) {
+   free_ep_req(hidg->out_ep, req);
return ret;
+   }
+   } else {
+   spin_lock_irqsave(&hidg->spinlock, flags);
+   list_add(&list->list, &hidg->completed_out_req);
+   spin_unlock_irqrestore(&hidg->spinlock, flags);
+
+   wake_up(&hidg->read_queue);
}
 
return count;
@@ -506,14 +518,18 @@ static void hidg_disable(struct usb_function *f)
 {
struct f_hidg *hidg = func_to_hidg(f);
struct f_hidg_req_list *list, *next;
+   unsigned long flags;
 
usb_ep_disable(hidg->in_ep);
usb_ep_disable(hidg->out_ep);
 
+   spin_lock_irqsave(&hidg->spinlock, flags);
list_for_each_entry_safe(list, next, &hidg->completed_out_req, list) {
+   free_ep_req(hidg->out_ep, list->req);
list_del(&list->list);
kfree(list);
}
+   spin_unlock_irqrestore(&hidg->spinlock, flags);
 }
 
 static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
-- 
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

[PATCH 3/4] usb: gadget: f_hid: Use spinlock instead of mutex

As IN request has to be allocated in set_alt() and released in
disable() we cannot use mutex to protect it as we cannot sleep
in those funcitons. Let's replace this mutex with a spinlock.

Cc: sta...@vger.kernel.org
Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/function/f_hid.c | 57 ++---
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c 
b/drivers/usb/gadget/function/f_hid.c
index b914e57..b0f7195 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -50,12 +50,12 @@ struct f_hidg {
 
/* recv report */
struct list_headcompleted_out_req;
-   spinlock_t  spinlock;
+   spinlock_t  read_spinlock;
wait_queue_head_t   read_queue;
unsigned intqlen;
 
/* send report */
-   struct mutexlock;
+   spinlock_t  write_spinlock;
boolwrite_pending;
wait_queue_head_t   write_queue;
struct usb_request  *req;
@@ -204,20 +204,20 @@ static ssize_t f_hidg_read(struct file *file, char __user 
*buffer,
if (!access_ok(VERIFY_WRITE, buffer, count))
return -EFAULT;
 
-   spin_lock_irqsave(&hidg->spinlock, flags);
+   spin_lock_irqsave(&hidg->read_spinlock, flags);
 
 #define READ_COND (!list_empty(&hidg->completed_out_req))
 
/* wait for at least one buffer to complete */
while (!READ_COND) {
-   spin_unlock_irqrestore(&hidg->spinlock, flags);
+   spin_unlock_irqrestore(&hidg->read_spinlock, flags);
if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
 
if (wait_event_interruptible(hidg->read_queue, READ_COND))
return -ERESTARTSYS;
 
-   spin_lock_irqsave(&hidg->spinlock, flags);
+   spin_lock_irqsave(&hidg->read_spinlock, flags);
}
 
/* pick the first one */
@@ -232,7 +232,7 @@ static ssize_t f_hidg_read(struct file *file, char __user 
*buffer,
 
req = list->req;
count = min_t(unsigned int, count, req->actual - list->pos);
-   spin_unlock_irqrestore(&hidg->spinlock, flags);
+   spin_unlock_irqrestore(&hidg->read_spinlock, flags);
 
/* copy to user outside spinlock */
count -= copy_to_user(buffer, req->buf + list->pos, count);
@@ -254,9 +254,9 @@ static ssize_t f_hidg_read(struct file *file, char __user 
*buffer,
return ret;
}
} else {
-   spin_lock_irqsave(&hidg->spinlock, flags);
+   spin_lock_irqsave(&hidg->read_spinlock, flags);
list_add(&list->list, &hidg->completed_out_req);
-   spin_unlock_irqrestore(&hidg->spinlock, flags);
+   spin_unlock_irqrestore(&hidg->read_spinlock, flags);
 
wake_up(&hidg->read_queue);
}
@@ -267,13 +267,16 @@ static ssize_t f_hidg_read(struct file *file, char __user 
*buffer,
 static void f_hidg_req_complete(struct usb_ep *ep, struct usb_request *req)
 {
struct f_hidg *hidg = (struct f_hidg *)ep->driver_data;
+   unsigned long flags;
 
if (req->status != 0) {
ERROR(hidg->func.config->cdev,
"End Point Request ERROR: %d\n", req->status);
}
 
+   spin_lock_irqsave(&hidg->write_spinlock, flags);
hidg->write_pending = 0;
+   spin_unlock_irqrestore(&hidg->write_spinlock, flags);
wake_up(&hidg->write_queue);
 }
 
@@ -281,18 +284,19 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
size_t count, loff_t *offp)
 {
struct f_hidg *hidg  = file->private_data;
+   unsigned long flags;
ssize_t status = -ENOMEM;
 
if (!access_ok(VERIFY_READ, buffer, count))
return -EFAULT;
 
-   mutex_lock(&hidg->lock);
+   spin_lock_irqsave(&hidg->write_spinlock, flags);
 
 #define WRITE_COND (!hidg->write_pending)
 
/* write queue */
while (!WRITE_COND) {
-   mutex_unlock(&hidg->lock);
+   spin_unlock_irqrestore(&hidg->write_spinlock, flags);
if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
 
@@ -300,17 +304,20 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
hidg->write_queue, WRITE_COND))
return -ERESTARTSYS;
 
-   mutex_lock(&hidg->lock);
+   spin_lock_irqsave(&

[PATCH 4/4] usb: gadget: f_hid: fix: Move IN request allocation to set_alt()

Since commit ba1582f22231 ("usb: gadget: f_hid: use alloc_ep_req()")
we cannot allocate any requests in bind() as we check if we should
align request buffer based on endpoint descriptor which is assigned
in set_alt().

Allocating request in bind() function causes a NULL pointer
dereference.

This commit moves allocation of IN request from bind() to set_alt()
to prevent this issue.

Fixes: ba1582f22231 ("usb: gadget: f_hid: use alloc_ep_req()")
Cc: sta...@vger.kernel.org
Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/function/f_hid.c | 89 -
 1 file changed, 67 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c 
b/drivers/usb/gadget/function/f_hid.c
index b0f7195..05b1206 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -284,6 +284,7 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
size_t count, loff_t *offp)
 {
struct f_hidg *hidg  = file->private_data;
+   struct usb_request *req;
unsigned long flags;
ssize_t status = -ENOMEM;
 
@@ -293,7 +294,7 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
spin_lock_irqsave(&hidg->write_spinlock, flags);
 
 #define WRITE_COND (!hidg->write_pending)
-
+try_again:
/* write queue */
while (!WRITE_COND) {
spin_unlock_irqrestore(&hidg->write_spinlock, flags);
@@ -308,6 +309,7 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
}
 
hidg->write_pending = 1;
+   req = hidg->req;
count  = min_t(unsigned, count, hidg->report_length);
 
spin_unlock_irqrestore(&hidg->write_spinlock, flags);
@@ -320,24 +322,38 @@ static ssize_t f_hidg_write(struct file *file, const char 
__user *buffer,
goto release_write_pending;
}
 
-   hidg->req->status   = 0;
-   hidg->req->zero = 0;
-   hidg->req->length   = count;
-   hidg->req->complete = f_hidg_req_complete;
-   hidg->req->context  = hidg;
+   spin_lock_irqsave(&hidg->write_spinlock, flags);
+
+   /* we our function has been disabled by host */
+   if (!hidg->req) {
+   free_ep_req(hidg->in_ep, hidg->req);
+   /*
+* TODO
+* Should we fail with error here?
+*/
+   goto try_again;
+   }
+
+   req->status   = 0;
+   req->zero = 0;
+   req->length   = count;
+   req->complete = f_hidg_req_complete;
+   req->context  = hidg;
 
status = usb_ep_queue(hidg->in_ep, hidg->req, GFP_ATOMIC);
if (status < 0) {
ERROR(hidg->func.config->cdev,
"usb_ep_queue error on int endpoint %zd\n", status);
-   goto release_write_pending;
+   goto release_write_pending_unlocked;
} else {
status = count;
}
+   spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 
return status;
 release_write_pending:
spin_lock_irqsave(&hidg->write_spinlock, flags);
+release_write_pending_unlocked:
hidg->write_pending = 0;
spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 
@@ -541,12 +557,23 @@ static void hidg_disable(struct usb_function *f)
kfree(list);
}
spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+
+   spin_lock_irqsave(&hidg->write_spinlock, flags);
+   if (!hidg->write_pending) {
+   free_ep_req(hidg->in_ep, hidg->req);
+   hidg->write_pending = 1;
+   }
+
+   hidg->req = NULL;
+   spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 }
 
 static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 {
struct usb_composite_dev*cdev = f->config->cdev;
struct f_hidg   *hidg = func_to_hidg(f);
+   struct usb_request  *req_in = NULL;
+   unsigned long   flags;
int i, status = 0;
 
VDBG(cdev, "hidg_set_alt intf:%d alt:%d\n", intf, alt);
@@ -567,6 +594,12 @@ static int hidg_set_alt(struct usb_function *f, unsigned 
intf, unsigned alt)
goto fail;
}
hidg->in_ep->driver_data = hidg;
+
+   req_in = hidg_alloc_ep_req(hidg->in_ep, hidg->report_length);
+   if (!req_in) {
+   status = -ENOMEM;
+   goto disable_ep_in;
+   }
}
 
 
@@ -578,12 +611,12 @@ static int hidg_set_alt(struct usb_function *f, unsigned 
intf, unsigned alt)

[PATCH 1/4] usb: gadget: f_hid: fix: Free out requests

Requests for out endpoint are allocated in bind() function
but never released.

This commit ensures that all pending requests are released
when we disable out endpoint.

Fixes: 99c515005857 ("usb: gadget: hidg: register OUT INT endpoint for 
SET_REPORT")
Cc: sta...@vger.kernel.org
Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/function/f_hid.c | 32 
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c 
b/drivers/usb/gadget/function/f_hid.c
index e2966f8..ceb1d0c 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -371,20 +371,36 @@ static inline struct usb_request 
*hidg_alloc_ep_req(struct usb_ep *ep,
 static void hidg_set_report_complete(struct usb_ep *ep, struct usb_request 
*req)
 {
struct f_hidg *hidg = (struct f_hidg *) req->context;
+   struct usb_composite_dev *cdev = hidg->func.config->cdev;
struct f_hidg_req_list *req_list;
unsigned long flags;
 
-   req_list = kzalloc(sizeof(*req_list), GFP_ATOMIC);
-   if (!req_list)
-   return;
+   switch (req->status) {
+   case 0:
+   req_list = kzalloc(sizeof(*req_list), GFP_ATOMIC);
+   if (!req_list) {
+   ERROR(cdev, "Unable to allocate mem for req_list\n");
+   goto free_req;
+   }
 
-   req_list->req = req;
+   req_list->req = req;
 
-   spin_lock_irqsave(&hidg->spinlock, flags);
-   list_add_tail(&req_list->list, &hidg->completed_out_req);
-   spin_unlock_irqrestore(&hidg->spinlock, flags);
+   spin_lock_irqsave(&hidg->spinlock, flags);
+   list_add_tail(&req_list->list, &hidg->completed_out_req);
+   spin_unlock_irqrestore(&hidg->spinlock, flags);
 
-   wake_up(&hidg->read_queue);
+   wake_up(&hidg->read_queue);
+   break;
+   default:
+   ERROR(cdev, "Set report failed %d\n", req->status);
+   /* FALLTHROUGH */
+   case -ECONNABORTED: /* hardware forced ep reset */
+   case -ECONNRESET:   /* request dequeued */
+   case -ESHUTDOWN:/* disconnect from host */
+free_req:
+   free_ep_req(ep, req);
+   return;
+   }
 }
 
 static int hidg_setup(struct usb_function *f,
-- 
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

[PATCH v3] usb: gadget: udc-core: Rescan pending list on driver unbind

2017-01-15 Thread Krzysztof Opasiak

Since:

commit 855ed04a3758 ("usb: gadget: udc-core: independent registration
of gadgets and gadget drivers")

if we load gadget module but there is no free udc available
then it will be stored on a pending gadgets list.

$ modprobe g_zero.ko
$ modprobe g_ether.ko
[] udc-core: couldn't find an available UDC - added [g_ether] to list
of pending drivers

We scan this list each time when new UDC appears in system.
But we can get a free UDC each time after gadget unbind.
This commit add scanning of that list directly after unbinding
gadget from udc.

Thanks to this, when we unload first gadget:

$ rmmod g_zero.ko

gadget which is pending is automatically
attached to that UDC (if name matches).

Fixes: 855ed04a3758  ("usb: gadget: udc-core: independent registration of 
gadgets and gadget drivers")
Cc: stable 
Signed-off-by: Krzysztof Opasiak 
---
Changes since v2:
- cc stable
Changes since v1:
- remove unused variable driver
---
 drivers/usb/gadget/udc/core.c | 45 +--
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 9483489080f6..73292459acd6 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1080,6 +1080,24 @@ static void usb_udc_nop_release(struct device *dev)
dev_vdbg(dev, "%s\n", __func__);
 }
 
+/* should be called with udc_lock held */
+static int check_pending_gadget_drivers(struct usb_udc *udc)
+{
+   struct usb_gadget_driver *driver;
+   int ret = 0;
+
+   list_for_each_entry(driver, &gadget_driver_pending_list, pending)
+   if (!driver->udc_name || strcmp(driver->udc_name,
+   dev_name(&udc->dev)) == 0) {
+   ret = udc_bind_to_driver(udc, driver);
+   if (ret != -EPROBE_DEFER)
+   list_del(&driver->pending);
+   break;
+   }
+
+   return ret;
+}
+
 /**
  * usb_add_gadget_udc_release - adds a new gadget to the udc class driver list
  * @parent: the parent device to this udc. Usually the controller driver's
@@ -1093,7 +,6 @@ int usb_add_gadget_udc_release(struct device *parent, 
struct usb_gadget *gadget,
void (*release)(struct device *dev))
 {
struct usb_udc  *udc;
-   struct usb_gadget_driver *driver;
int ret = -ENOMEM;
 
udc = kzalloc(sizeof(*udc), GFP_KERNEL);
@@ -1136,17 +1153,9 @@ int usb_add_gadget_udc_release(struct device *parent, 
struct usb_gadget *gadget,
udc->vbus = true;
 
/* pick up one of pending gadget drivers */
-   list_for_each_entry(driver, &gadget_driver_pending_list, pending) {
-   if (!driver->udc_name || strcmp(driver->udc_name,
-   dev_name(&udc->dev)) == 0) {
-   ret = udc_bind_to_driver(udc, driver);
-   if (ret != -EPROBE_DEFER)
-   list_del(&driver->pending);
-   if (ret)
-   goto err5;
-   break;
-   }
-   }
+   ret = check_pending_gadget_drivers(udc);
+   if (ret)
+   goto err5;
 
mutex_unlock(&udc_lock);
 
@@ -1352,14 +1361,22 @@ int usb_gadget_unregister_driver(struct 
usb_gadget_driver *driver)
return -EINVAL;
 
mutex_lock(&udc_lock);
-   list_for_each_entry(udc, &udc_list, list)
+   list_for_each_entry(udc, &udc_list, list) {
if (udc->driver == driver) {
usb_gadget_remove_driver(udc);
usb_gadget_set_state(udc->gadget,
-   USB_STATE_NOTATTACHED);
+USB_STATE_NOTATTACHED);
+
+   /* Maybe there is someone waiting for this UDC? */
+   check_pending_gadget_drivers(udc);
+   /*
+* For now we ignore bind errors as probably it's
+* not a valid reason to fail other's gadget unbind
+*/
ret = 0;
break;
}
+   }
 
if (ret) {
list_del(&driver->pending);
-- 
2.9.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 v2 RESEND] usb: gadget: udc-core: Rescan pending list on driver unbind

2017-01-15 Thread Krzysztof Opasiak



On 12/27/2016 12:18 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Krzysztof Opasiak  writes:
>> Since:
>>
>> commit 855ed04a3758 ("usb: gadget: udc-core: independent registration
>> of gadgets and gadget drivers")
>>
>> if we load gadget module but there is no free udc available
>> then it will be stored on a pending gadgets list.
>>
>> $ modprobe g_zero.ko
>> $ modprobe g_ether.ko
>> [] udc-core: couldn't find an available UDC - added [g_ether] to list
>> of pending drivers
>>
>> We scan this list each time when new UDC appears in system.
>> But we can get a free UDC each time after gadget unbind.
>> This commit add scanning of that list directly after unbinding
>> gadget from udc.
>>
>> Thanks to this, when we unload first gadget:
>>
>> $ rmmod g_zero.ko
>>
>> gadget which is pending is automatically
>> attached to that UDC (if name matches).
>>
>> Signed-off-by: Krzysztof Opasiak 
> 
> Do we need to Cc stable on this?
> 

I'll resend this patch with suitable CC

-- 
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] tools: usb usbip - update README USB/IP driver location

2017-01-15 Thread Krzysztof Opasiak



On 01/14/2017 12:38 AM, Shuah Khan wrote:
> Update USB/IP driver location in README.
> 
> Signed-off-by: Shuah Khan 

Reviewed-by: Krzysztof Opasiak 

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: [PATCH v8 0/2] usbip: vhci number of ports extension



On 12/26/2016 08:18 AM, Nobuo Iwata wrote:
> This series of patches extends number of ports limitaion in application 
> (vhci) side.
> 

(...)

> 5. Dependencies
> 
> This series depends on 'usbip: exporting devices' patch set because 
> this includes changes to application side daemon which introduced the 
> patch set.

You should definitely revers the dependency as this series is way
simpler and requires much less discussion so in my humble opinion it has
much bigger chances of getting quick into the kernel.

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: [PATCH v14 00/10] usbip: exporting devices

On 12/26/2016 08:08 AM, Nobuo Iwata wrote:
> Dear all,
> 
> This series of patches adds exporting device operation to USB/IP.
> 

I run through most of your series and answered with couple of comments.

I'm not checking all patches in detail because I see that there is a lot
of remarks from previous series which you didn't applied nor reply to them.

Please apply those remarks or at least let's make some discussion about
them.

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: [PATCH v14 06/10] usbip: exporting devices: modifications to attach and detach

FIRST))
> + goto err_free_driver;
>  
>   vhci_driver->nports = get_nports();
>  
>   dbg("available ports: %d", vhci_driver->nports);
>  
>   if (refresh_imported_device_list())
> - goto err;
> + goto err_unref_device;
>  
>   return 0;
>  
> -err:
> +err_unref_device:
>   udev_device_unref(vhci_driver->hc_device);
> -
> +err_free_driver:
>   if (vhci_driver)
>   free(vhci_driver);
>  
> @@ -277,7 +291,8 @@ void usbip_vhci_driver_close(void)
>  
>  int usbip_vhci_refresh_device_list(void)
>  {
> -
> + if (open_hc_device(OPEN_HC_MODE_REOPEN))
> + goto err;

empty line

>   if (refresh_imported_device_list())
>   goto err;
>  
> @@ -409,3 +424,58 @@ int usbip_vhci_imported_device_dump(struct 
> usbip_imported_device *idev)
>  
>   return 0;
>  }
> +
> +#define MAX_BUFF 100
> +int usbip_vhci_create_record(char *host, char *port, char *busid, int rhport)
> +{
> + int fd;
> + char path[PATH_MAX+1];
> + char buff[MAX_BUFF+1];
> + int ret;
> +
> + ret = mkdir(VHCI_STATE_PATH, 0700);
> + if (ret < 0) {
> + /* if VHCI_STATE_PATH exists, then it better be a directory */
> + if (errno == EEXIST) {
> + struct stat s;
> +
> + ret = stat(VHCI_STATE_PATH, &s);
> + if (ret < 0)
> + return -1;
> + if (!(s.st_mode & S_IFDIR))
> + return -1;
> + } else
> + return -1;
> + }
> +
> + snprintf(path, PATH_MAX, VHCI_STATE_PATH"/port%d", rhport);
> +
> + fd = open(path, O_WRONLY|O_CREAT|O_TRUNC, 0700);
> + if (fd < 0)
> + return -1;
> +
> + snprintf(buff, MAX_BUFF, "%s %s %s\n",
> + host, port, busid);

if you use snprintf() it would be nice to check the returned value

> +
> + ret = write(fd, buff, strlen(buff));
> + if (ret != (ssize_t) strlen(buff)) {
> + close(fd);
> + return -1;
> + }
> +
> + close(fd);
> +
> + return 0;
> +}
> +
> +int usbip_vhci_delete_record(int rhport)
> +{
> + char path[PATH_MAX+1];
> +
> + snprintf(path, PATH_MAX, VHCI_STATE_PATH"/port%d", rhport);
> +
> + remove(path);
> + rmdir(VHCI_STATE_PATH);

it would be nice to add a comment here that you don't check the return
value because it will succeed only when all files had been removed

> +
> + return 0;
> +}
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.h 
> b/tools/usb/usbip/libsrc/vhci_driver.h
> index fa2316c..c85988c 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.h
> +++ b/tools/usb/usbip/libsrc/vhci_driver.h
> @@ -54,6 +54,9 @@ int usbip_vhci_attach_device(uint8_t port, int sockfd, 
> uint8_t busnum,
>  
>  int usbip_vhci_detach_device(uint8_t port);
>  
> +int usbip_vhci_create_record(char *host, char *port, char *busid, int 
> rhport);
> +int usbip_vhci_delete_record(int rhport);
> +
>  int usbip_vhci_imported_device_dump(struct usbip_imported_device *idev);
>  
>  #endif /* __VHCI_DRIVER_H */
> diff --git a/tools/usb/usbip/src/usbip_attach.c 
> b/tools/usb/usbip/src/usbip_attach.c
> index 695b2e5..17a84ea 100644
> --- a/tools/usb/usbip/src/usbip_attach.c
> +++ b/tools/usb/usbip/src/usbip_attach.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2015-2016 Samsung Electronics
>   *   Igor Kotrasinski 
>   *   Krzysztof Opasiak 
> + * Copyright (C) 2015-2016 Nobuo Iwata 
>   *
>   * This program is free software: you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -26,7 +27,6 @@
>  #include 
>  #include 
>  
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -47,83 +47,53 @@ void usbip_attach_usage(void)
>   printf("usage: %s", usbip_attach_usage_string);
>  }
>  
> -#define MAX_BUFF 100
> -static int record_connection(char *host, char *port, char *busid, int rhport)
> -{
> - int fd;
> - char path[PATH_MAX+1];
> - char buff[MAX_BUFF+1];
> - int ret;
> -
> - ret = mkdir(VHCI_STATE_PATH, 0700);
> - if (ret < 0) {
> - /* if VHCI_STATE_PATH exists, then it better be a directory */
> - if (errno == EEXIST) {
> - struct stat s;
> -
> - ret = stat(VHCI_STATE_PATH, &s);
> - if (ret < 0)
> -

Re: [PATCH v14 02/10] usbip: exporting devices: modifications to host side libraries



On 01/13/2017 04:46 PM, Shuah Khan wrote:
> On 01/13/2017 04:55 AM, Krzysztof Opasiak wrote:
>>
>>
>> On 01/12/2017 10:14 PM, Shuah Khan wrote:
>>> On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
>>>> Modifications to host driver wrapper and its related operations (i.e. 
>>>> bind/unbind).
>>>
>>> Way too many changes in this one patch.
>>>
>>>>
>>>> usbip_get_device() method was not used. The implementation of the 
>>>> method searches a bound devices list by list index. The position in the 
>>>> list is meaningless for any operations so it is assumed not to be used 
>>>> in future.
>>>>
>>>> For this patch set, a method to find a bound device in the bound 
>>>> devices list by bus-id is needed. usbip_get_device() is re-implemented 
>>>> as a function to find a bound device by bus-id.
>>>
>>> This is not an accurate description. You are really changing look ups using
>>> bus-id instead bus-num and whether usbip_get_device() is used or not is
>>> irrelevant.
>>>
>>> Please make changes to find bound device by bus-id in a separate patch.
>>> You will have to change usbip_get_device() anyway because you are changing
>>> .get_device() interface. Include exporting usbip_get_debice() in a separate
>>> patch.
>>>
>>>>
>>>> bind and unbind function are exported to reuse by new connect and 
>>>> disconnect operation. Here, connect and disconnect is NEW-3 and NEW-4 
>>>> respactively in diagram below.
>>>
>>> Please don't include exporting bind_device() and unbind_device() and
>>> changing their names in this patch. Send a separate patch for that.
>>>
>>> It makes sense to move the functions you are exporting bind_device(),
>>> unbind_device(), and usbip_get_device() to libsrc - usbip_common.c
>>> might be a good choice.
>>>
>>> The following isn't part of this patch, hence shouldn't be included in
>>> the change log.
>>>
>>>>
>>>> EXISTING) - invites devices from application(vhci)-side
>>>>  +--+   +--+
>>>>  device--+ STUB |   | application/VHCI |
>>>>  +--+   +--+
>>>>  (server)   (client)
>>>>  1) # usbipd ... start daemon
>>>>  = = =
>>>>  2) # usbip list --local
>>>>  3) # usbip bind
>>>>   <--- list bound devices ---  4) # usbip list --remote
>>>>   <--- import a device --  5) # usbip attach
>>>>  = = =
>>>>  X disconnected6) # usbip detach
>>>>  7) usbip unbind
>>>>
>>>> NEW) - dedicates devices from device(stub)-side
>>>>  +--+   +--+
>>>>  device--+ STUB |   | application/VHCI |
>>>>  +--+   +--+
>>>>  (client)   (server)
>>>> 1) # usbipa ... start daemon
>>>>  = = =
>>>>  2) # usbip list --local
>>>>  3) # usbip connect--- export a device -->
>>>>  = = =
>>>>  4) # usbip disconnect --- un-export a device --->
>>>>
>>>>  Bind and unbind are done in connect and disconnect internally.
>>>>
>>>> Signed-off-by: Nobuo Iwata 
>>>> ---
>>>>  tools/usb/usbip/libsrc/usbip_host_common.c | 6 ++
>>>>  tools/usb/usbip/libsrc/usbip_host_common.h | 8 
>>>>  tools/usb/usbip/src/usbip.h| 3 +++
>>>>  tools/usb/usbip/src/usbip_bind.c   | 4 ++--
>>>>  tools/usb/usbip/src/usbip_unbind.c | 4 ++--
>>>>  5 files changed, 13 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c 
>>>> b/tools/usb/usbip/libsrc/usbip_host_common.c
>>>> index 9d41522..6a98d6c 100644
>>>> --- a/tools/usb/usbip/libsrc/usbip_host_common.c
>>>> +++ b/tools/usb/usbip/libsrc/usbip_host_common.c
>>>> @@ -256,17 +256,15 @@ int usbip_export_device(struct usbip_exported_device 
>>>> *edev, int sockfd)
>>>>  }
>

Re: [PATCH v14 02/10] usbip: exporting devices: modifications to host side libraries



On 01/12/2017 10:14 PM, Shuah Khan wrote:
> On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
>> Modifications to host driver wrapper and its related operations (i.e. 
>> bind/unbind).
> 
> Way too many changes in this one patch.
> 
>>
>> usbip_get_device() method was not used. The implementation of the 
>> method searches a bound devices list by list index. The position in the 
>> list is meaningless for any operations so it is assumed not to be used 
>> in future.
>>
>> For this patch set, a method to find a bound device in the bound 
>> devices list by bus-id is needed. usbip_get_device() is re-implemented 
>> as a function to find a bound device by bus-id.
> 
> This is not an accurate description. You are really changing look ups using
> bus-id instead bus-num and whether usbip_get_device() is used or not is
> irrelevant.
> 
> Please make changes to find bound device by bus-id in a separate patch.
> You will have to change usbip_get_device() anyway because you are changing
> .get_device() interface. Include exporting usbip_get_debice() in a separate
> patch.
> 
>>
>> bind and unbind function are exported to reuse by new connect and 
>> disconnect operation. Here, connect and disconnect is NEW-3 and NEW-4 
>> respactively in diagram below.
> 
> Please don't include exporting bind_device() and unbind_device() and
> changing their names in this patch. Send a separate patch for that.
> 
> It makes sense to move the functions you are exporting bind_device(),
> unbind_device(), and usbip_get_device() to libsrc - usbip_common.c
> might be a good choice.
> 
> The following isn't part of this patch, hence shouldn't be included in
> the change log.
> 
>>
>> EXISTING) - invites devices from application(vhci)-side
>>  +--+   +--+
>>  device--+ STUB |   | application/VHCI |
>>  +--+   +--+
>>  (server)   (client)
>>  1) # usbipd ... start daemon
>>  = = =
>>  2) # usbip list --local
>>  3) # usbip bind
>>   <--- list bound devices ---  4) # usbip list --remote
>>   <--- import a device --  5) # usbip attach
>>  = = =
>>  X disconnected6) # usbip detach
>>  7) usbip unbind
>>
>> NEW) - dedicates devices from device(stub)-side
>>  +--+   +--+
>>  device--+ STUB |   | application/VHCI |
>>  +--+   +--+
>>  (client)   (server)
>> 1) # usbipa ... start daemon
>>  = = =
>>  2) # usbip list --local
>>  3) # usbip connect--- export a device -->
>>  = = =
>>  4) # usbip disconnect --- un-export a device --->
>>
>>  Bind and unbind are done in connect and disconnect internally.
>>
>> Signed-off-by: Nobuo Iwata 
>> ---
>>  tools/usb/usbip/libsrc/usbip_host_common.c | 6 ++
>>  tools/usb/usbip/libsrc/usbip_host_common.h | 8 
>>  tools/usb/usbip/src/usbip.h| 3 +++
>>  tools/usb/usbip/src/usbip_bind.c   | 4 ++--
>>  tools/usb/usbip/src/usbip_unbind.c | 4 ++--
>>  5 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c 
>> b/tools/usb/usbip/libsrc/usbip_host_common.c
>> index 9d41522..6a98d6c 100644
>> --- a/tools/usb/usbip/libsrc/usbip_host_common.c
>> +++ b/tools/usb/usbip/libsrc/usbip_host_common.c
>> @@ -256,17 +256,15 @@ int usbip_export_device(struct usbip_exported_device 
>> *edev, int sockfd)
>>  }
>>  
>>  struct usbip_exported_device *usbip_generic_get_device(
>> -struct usbip_host_driver *hdriver, int num)
>> +struct usbip_host_driver *hdriver, char *busid)
>>  {
>>  struct list_head *i;
>>  struct usbip_exported_device *edev;
>> -int cnt = 0;
>>  
>>  list_for_each(i, &hdriver->edev_list) {
>>  edev = list_entry(i, struct usbip_exported_device, node);
>> -if (num == cnt)
>> +if (!strncmp(busid, edev->udev.busid, SYSFS_BUS_ID_SIZE))
> 
> Use strlen(edev->udev.busid) instead of SYSFS_BUS_ID_SIZE
> 

For me using here strlen() is pointless. strncpy() is here to protect
against unterminated string in edev->udev.busid. In my humble opinion,
instead of using strncpy() all the time I would rather just ensure that
this string is \0 terminated in read_usb_device().

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: [PATCH v14 03/10] usbip: exporting devices: new connect operation



On 01/13/2017 01:06 AM, Shuah Khan wrote:
> On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
>> Implementation of new connect operation. This is linked as a part of 
>> usbip command. With this patch, usbip command has following operations.
>>
>>  bind
>>  unbind
>>  list (local/remote)
>>  attach
>>  detach
>>  port
>>  connect ... this patch
> 
> Don't include existing commands. Just list the new one. Why do you call
> this connect. Isn't it really export - connect isn't accurate. Call it
> export/unexport.
> 
>>
>> In device side node, this binds a device internally, sends an export 
>> request and receives export response. The definition of the request and 
>> response are defined in original code: tools/usb/usbip/usbip_network.h 
>> but was not used. They are corresponding to NEW-3 in following diagram. 
>>
>> EXISTING) - invites devices from application(vhci)-side
>>  +--+   +--+
>>  device--+ STUB |   | application/VHCI |
>>  +--+   +--+
>>  (server)   (client)
>>  1) # usbipd ... start daemon
>>  = = =
>>  2) # usbip list --local
>>  3) # usbip bind
>>   <--- list bound devices ---  4) # usbip list --remote
>>   <--- import a device --  5) # usbip attach
>>  = = =
>>  X disconnected6) # usbip detach
>>  7) usbip unbind
>>
>> NEW) - dedicates devices from device(stub)-side
>>  +--+   +--+
>>  device--+ STUB |   | application/VHCI |
>>  +--+   +--+
>>  (client)   (server)
> 
> Make the left side server and right side client. I think you might be
> using server and client network terminology. I would like to see server
> as the system that has the device physically attached to.
> 
> 
> I still want to see server as the one that is connected to the device.
> It is just that in this new proposed model, server is exporting devices.
> 
> Also does usbip run here in user mode. Can any user run uspip and initiate
> export? If so, I don't think I can take this patch series. I don't like to
> see non root being able to export and/or make attached devices public.
> 

By default it's not possible. See my response to patch 9.

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: [PATCH v14 09/10] usbip: exporting devices: chage to documenattion



On 01/13/2017 01:17 AM, Shuah Khan wrote:
> On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
>> This patch adds function and usage of new connect operation, disconnect 
>> operation and application(vhci)-side daemon to README and manuals.
> 
> This should be the first patch for the series. That would have saved
> me lot of time. Please move this patch up.
> 
>>
>> At this point, the wording, 'server' and 'client' are ambiguous in 
>> several place.
>>
>> For existing attach command, the daemon runs device side machine and 
>> attach command is executed in application side machine. Then 'server' 
>> is used for device side and 'client' is for application side.
>>
>> For the new connect command, the daemon runs applications side machine 
>> and connect command is executed in device side machine. Now, 'server' 
>> and 'client' run in different machine than before.
>>
>> To avoid confusion, to represent things to be done in device side node 
>> by both command and daemon, words 'device-side' is used instead of 
>> 'server'. To represent things to be done is application side node by 
>> both command and daemon, 'applicationr-side' are used instead of 
>> 'client'.
>>
>> EXISTING) - invites devices from application(vhci)-side
>>  +--+   +--+
>>  device--+ STUB |   | application/VHCI |
>>  +--+   +--+
>>  (server)   (client)
>>  1) # usbipd ... start daemon
>>  = = =
>>  2) # usbip list --local
>>  3) # usbip bind
>>   <--- list bound devices ---  4) # usbip list --remote
>>   <--- import a device --  5) # usbip attach
>>  = = =
>>  X disconnected6) # usbip detach
>>  7) usbip unbind
>>
>> NEW) - dedicates devices from device(stub)-side
>>  +--+   +--+
>>  device--+ STUB |   | application/VHCI |
>>  +--+   +--+
>>  (client)   (server)
>> 1) # usbipa ... start daemon
> 
> Make the left side server and right side client. I think you might be
> using server and client network terminology. I would like to see server
> as the system that has the device physically attached to.
> 
> 
> I still want to see server as the one that is connected to the device.
> It is just that in this new proposed model, server is exporting devices.
> 
> Also does usbip run here in user mode. Can any user run uspip and initiate
> export? If so, I don't think I can take this patch series. I don't like to
> see non root being able to export and/or make attached devices public.
> 

usbip connect (export operation) needs the same permissions as usbip
bind because it uses the same kernel infrastructure.

To export a device you need to bind a stub driver to it which you do
using bind attribute in drivers directory which by default has permissions:

--w---  1 root root 4096 Dec 31 17:12 bind

usbipa also has to run as a root to accept device which someone would
like to system because it writes to attach attribute in vhci driver dir
which is declared as follows:

static DEVICE_ATTR(attach, S_IWUSR, NULL, store_attach);

So summing up to export or to accept export on usbipa side both sides
requires root permissions.

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: [PATCH v14 05/10] usbip: exporting devices: modifications to daemon

OP_REP_DEVLIST, ST_OK);
> + if (rc < 0) {
> + dbg("usbip_net_send_op_common failed: %#0x", OP_REP_DEVLIST);
> + return -1;
> + }
> + PACK_OP_DEVLIST_REPLY(1, &reply);
> +
> + rc = usbip_net_send(connfd, &reply, sizeof(reply));
> + if (rc < 0) {
> + dbg("usbip_net_send failed: %#0x", OP_REP_DEVLIST);
> + return -1;
> + }
> +
> + list_for_each(j, &driver->edev_list) {
> + edev = list_entry(j, struct usbip_exported_device, node);

The same as above s/j/node...

> + dump_usb_device(&edev->udev);
> + memcpy(&pdu_udev, &edev->udev, sizeof(pdu_udev));
> + usbip_net_pack_usb_device(1, &pdu_udev);
> +
> + rc = usbip_net_send(connfd, &pdu_udev, sizeof(pdu_udev));
> + if (rc < 0) {
> + dbg("usbip_net_send failed: pdu_udev");
> + return -1;
> + }
> +
> + for (i = 0; i < edev->udev.bNumInterfaces; i++) {
> + dump_usb_interface(&edev->uinf[i]);
> + memcpy(&pdu_uinf, &edev->uinf[i], sizeof(pdu_uinf));
> + usbip_net_pack_usb_interface(1, &pdu_uinf);
> +
> + rc = usbip_net_send(connfd, &pdu_uinf,
> + sizeof(pdu_uinf));
> + if (rc < 0) {
> + err("usbip_net_send failed: pdu_uinf");
> + return -1;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int recv_request_devlist(int connfd)
> +{
> + int rc;
> +
> + rc = send_reply_devlist(connfd);
> + if (rc < 0) {
> + dbg("send_reply_devlist failed");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +struct usbipd_recv_pdu_op usbipd_recv_pdu_ops[] = {
> + {OP_REQ_DEVLIST, recv_request_devlist},
> + {OP_REQ_IMPORT, recv_request_import},
> + {OP_REQ_DEVINFO, NULL},
> + {OP_REQ_CRYPKEY, NULL},
> + {OP_UNSPEC, NULL}
> +};
> 

-- 
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 v14 04/10] usbip: exporting devices: new disconnect operation



On 12/26/2016 08:08 AM, Nobuo Iwata wrote:
> Implementation of new disconnect operation. This is linked as a part of 
> usbip command. With this patch, usbip command has following operations.
> 
>  bind
>  unbind
>  list (local/remote)
>  attach
>  detach
>  port
>  connect ... previous patch
>  disconnect ... this patch
> 
> In device side node, this sends an un-export request, receives an 
> un-export response and unbind corresponding device internally. The 
> definition of the request and response are defined in original code: 
> tools/usb/usbip/usbip_network.h but was not used. It's corresponding to 
> NEW-4 in following diagram.
> 
> To find disconnecting device, requesting host and requested 
> bus-id-in-requester identifies the target. So it cannot to disconnect a 
> device from other host than a host which connected the device. 
> 
> EXISTING) - invites devices from application(vhci)-side
>  +--+   +--+
>  device--+ STUB |   | application/VHCI |
>  +--+   +--+
>  (server)   (client)
>  1) # usbipd ... start daemon
>  = = =
>  2) # usbip list --local
>  3) # usbip bind
>   <--- list bound devices ---  4) # usbip list --remote
>   <--- import a device --  5) # usbip attach
>  = = =
>  X disconnected6) # usbip detach
>  7) usbip unbind
> 
> NEW) - dedicates devices from device(stub)-side
>  +--+   +--+
>  device--+ STUB |   | application/VHCI |
>  +--+   +--+
>  (client)   (server)
> 1) # usbipa ... start daemon
>  = = =
>  2) # usbip list --local
>  3) # usbip connect--- export a device -->
>  = = =
>  4) # usbip disconnect --- un-export a device --->
> 
>  Bind and unbind are done in connect and disconnect internally.

The same as in previous commit. Please clarify that those operations are
done only for stub driver.

(...)

> +static int disconnect_device(char *host, char *busid, int unbind)
> +{
> + int sockfd;
> + int rc;
> +
> + sockfd = usbip_net_tcp_connect(host, usbip_port_string);
> + if (sockfd < 0) {
> + err("tcp connect");
> + return -1;
> + }
> +
> + rc = unexport_device(busid, sockfd);
> + if (rc < 0) {
> + err("unexport");
> + close(sockfd);
> + return -1;
> + }
> +
> + close(sockfd);

My comment from previous version (still not fixed):

>> +
>> +rc = unexport_device(busid, sockfd);
>> +if (rc < 0) {
>> +err("unexport");
>> +close(sockfd);
>> +return -1;
>> +}
>
> close(sockfd) in case of error, otherwise close(sockfd)...
>
> Probably you can place close(sockfd) above if to avoid this weird
> copy-paste.
>
>> +
>> +close(sockfd);
>> +


> +
> + if (unbind) {
> + rc = usbip_unbind_device(busid);
> + if (rc) {
> + err("unbind");
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +int usbip_disconnect(int argc, char *argv[])
> +{
> + static const struct option opts[] = {
> + { "remote", required_argument, NULL, 'r' },
> + { "busid",  required_argument, NULL, 'b' },
> + { "device", no_argument,   NULL, 'd' },
> + { NULL, 0,  NULL, 0 }
> + };
> + char *host = NULL;
> + char *busid = NULL;
> + int opt;
> + int unbind = 1;

Shouldn't this parameter be exposed to cmd line among remote, busid and
device? User may want to disconnect device but leave stub driver bound
to it.

> + int ret = -1;
> +
> + for (;;) {
> + opt = getopt_long(argc, argv, "r:b:d", opts, NULL);
> +
> + if (opt == -1)
> + break;
> +
> + switch (opt) {
> + case 'r':
> + host = optarg;
> + break;
> + case 'b':
> +     busid = optarg;
> + break;
> + case 'd':
> + driver = &device_

Re: [PATCH v14 03/10] usbip: exporting devices: new connect operation

   rc = usbip_bind_device(busid);
> + if (rc) {
> + err("bind");
> + goto err_out;
> + }
> + }
> +
> + sockfd = usbip_net_tcp_connect(host, usbip_port_string);
> + if (sockfd < 0) {
> + err("tcp connect");
> + goto err_unbind_device;
> + }
> +
> + rc = export_device(busid, sockfd);
> + if (rc < 0) {
> + err("export");
> + goto err_close_conn;
> + }
> +
> + close(sockfd);
> +
> + return 0;
> +err_close_conn:
> + close(sockfd);
> +err_unbind_device:
> + if (bind)
> + usbip_unbind_device(busid);
> +err_out:
> + return -1;
> +}
> +
> +int usbip_connect(int argc, char *argv[])
> +{
> + static const struct option opts[] = {
> + { "remote", required_argument, NULL, 'r' },
> + { "busid",  required_argument, NULL, 'b' },
> +         { "device", no_argument,   NULL, 'd' },
> + { NULL, 0,  NULL, 0 }
> + };
> + char *host = NULL;
> + char *busid = NULL;
> + int opt;
> + int bind = 1;

Shouldn't this parameter be exposed to cmd line among remote, busid and
device?

> + int ret = -1;
> +
> + for (;;) {
> + opt = getopt_long(argc, argv, "r:b:d", opts, NULL);
> +
> + if (opt == -1)
> + break;
> +
> + switch (opt) {
> + case 'r':
> + host = optarg;
> + break;
> + case 'b':
> + busid = optarg;
> + break;
> + case 'd':
> + driver = &device_driver;
> + bind = 0;
> + break;
> + default:
> + goto err_out;
> + }
> + }
> +
> + if (!host || !busid)
> + goto err_out;
> +
> + ret = connect_device(host, busid, bind);
> + goto out;
> +
> +err_out:
> + usbip_connect_usage();
> +out:
> + return ret;
> +}
> 

This looks far better than previous edition. Thank you!
Just a couple of minor issues and should be ok.

Cheers,
-- 
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 v14 02/10] usbip: exporting devices: modifications to host side libraries



On 12/26/2016 08:08 AM, Nobuo Iwata wrote:
> Modifications to host driver wrapper and its related operations (i.e. 
> bind/unbind).
> 
> usbip_get_device() method was not used. The implementation of the 
> method searches a bound devices list by list index. The position in the 
> list is meaningless for any operations so it is assumed not to be used 
> in future.
> 
> For this patch set, a method to find a bound device in the bound 
> devices list by bus-id is needed. usbip_get_device() is re-implemented 
> as a function to find a bound device by bus-id.

Great. Now make this a separate patch and send separated from this series.

PS. remember to fix title for this commit.

> 
> bind and unbind function are exported to reuse by new connect and 
> disconnect operation. Here, connect and disconnect is NEW-3 and NEW-4 
> respactively in diagram below.

s/respactively/respectively

and then this could be a commit titled:

usbip: Make bind and unbind helpers global functions

> 
> EXISTING) - invites devices from application(vhci)-side
>  +--+   +--+
>  device--+ STUB |   | application/VHCI |
>  +--+   +--+
>  (server)   (client)
>  1) # usbipd ... start daemon
>  = = =
>  2) # usbip list --local
>  3) # usbip bind
>   <--- list bound devices ---  4) # usbip list --remote
>   <--- import a device --  5) # usbip attach
>  = = =
>  X disconnected6) # usbip detach
>  7) usbip unbind
> 
> NEW) - dedicates devices from device(stub)-side
>  +--+   +--+
>  device--+ STUB |   | application/VHCI |
>  +--+   +--+
>  (client)   (server)
> 1) # usbipa ... start daemon
>  = = =
>  2) # usbip list --local
>  3) # usbip connect--- export a device -->
>  = = =
>  4) # usbip disconnect --- un-export a device --->
> 
>  Bind and unbind are done in connect and disconnect internally.
> 
> Signed-off-by: Nobuo Iwata 
> ---
>  tools/usb/usbip/libsrc/usbip_host_common.c | 6 ++
>  tools/usb/usbip/libsrc/usbip_host_common.h | 8 
>  tools/usb/usbip/src/usbip.h| 3 +++
>  tools/usb/usbip/src/usbip_bind.c   | 4 ++--
>  tools/usb/usbip/src/usbip_unbind.c | 4 ++--
>  5 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c 
> b/tools/usb/usbip/libsrc/usbip_host_common.c
> index 9d41522..6a98d6c 100644
> --- a/tools/usb/usbip/libsrc/usbip_host_common.c
> +++ b/tools/usb/usbip/libsrc/usbip_host_common.c
> @@ -256,17 +256,15 @@ int usbip_export_device(struct usbip_exported_device 
> *edev, int sockfd)
>  }
>  
>  struct usbip_exported_device *usbip_generic_get_device(
> - struct usbip_host_driver *hdriver, int num)
> + struct usbip_host_driver *hdriver, char *busid)
>  {
>   struct list_head *i;
>   struct usbip_exported_device *edev;
> - int cnt = 0;
>  
>   list_for_each(i, &hdriver->edev_list) {
>   edev = list_entry(i, struct usbip_exported_device, node);
> - if (num == cnt)
> + if (!strncmp(busid, edev->udev.busid, SYSFS_BUS_ID_SIZE))

does it really make sense to use strncmp() instead of strcmp() here?

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: [PATCH v14 01/10] usbip: exporting devices: modifications to network header



On 12/26/2016 08:08 AM, Nobuo Iwata wrote:
> This patch set implements operations to export and unexport device 
> using pre-defined but un-used export and un-export request and reply.
> 
> This patch modifies export and un-export reply in 
> tools/usb/usbip/src/usbip_network.h. The 'returncode' of each response 
> is removed. The status is set in op_common.status.
> 
> Following status codes are added for this patch set.
>   ST_NO_FREE_POR
>   ST_NOT_FOUND
> 
> The reason to use op_common.status:
>   1) usbip_net_recv_op_common() hanles as error when the status is other
>  than ST_OK.
>   2) The status has a commnet "add more error code".
> 
> They become empty struct. Other empty struct, 'op_devlist_request', 
> defined. 
> 
> This patch also includes string translation of the status codes.
> 
> Signed-off-by: Nobuo Iwata 

Reviewied-by: Krzysztof Opasiak 

> ---
>  tools/usb/usbip/src/usbip_network.c | 26 +-
>  tools/usb/usbip/src/usbip_network.h |  8 
>  2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/usb/usbip/src/usbip_network.c 
> b/tools/usb/usbip/src/usbip_network.c
> index b4c37e7..069ec54 100644
> --- a/tools/usb/usbip/src/usbip_network.c
> +++ b/tools/usb/usbip/src/usbip_network.c
> @@ -163,6 +163,29 @@ int usbip_net_send_op_common(int sockfd, uint32_t code, 
> uint32_t status)
>   return 0;
>  }
>  
> +struct op_common_error {
> + uint32_tstatus;
> + const char  *str;
> +};
> +
> +struct op_common_error op_common_errors[] = {
> + {ST_NA, "not available"},
> + {ST_NO_FREE_PORT, "no free port"},
> + {ST_DEVICE_NOT_FOUND, "device not found"},
> + {0, NULL}
> +};
> +
> +static const char *op_common_strerror(uint32_t status)
> +{
> + struct op_common_error *err;
> +
> + for (err = op_common_errors; err->str != NULL; err++) {
> + if (err->status == status)
> + return err->str;
> + }
> + return "unknown error";
> +}
> +
>  int usbip_net_recv_op_common(int sockfd, uint16_t *code)
>  {
>   struct op_common op_common;
> @@ -196,7 +219,8 @@ int usbip_net_recv_op_common(int sockfd, uint16_t *code)
>   }
>  
>   if (op_common.status != ST_OK) {
> - dbg("request failed at peer: %d", op_common.status);
> + dbg("request failed at peer: %s",
> + op_common_strerror(op_common.status));
>   goto err;
>   }
>  
> diff --git a/tools/usb/usbip/src/usbip_network.h 
> b/tools/usb/usbip/src/usbip_network.h
> index c1e875c..86c3ff0 100644
> --- a/tools/usb/usbip/src/usbip_network.h
> +++ b/tools/usb/usbip/src/usbip_network.h
> @@ -27,8 +27,10 @@ struct op_common {
>   uint16_t code;
>  
>   /* add more error code */
> -#define ST_OK0x00
> -#define ST_NA0x01
> +#define ST_OK0x00
> +#define ST_NA0x01
> +#define ST_NO_FREE_PORT  0x02
> +#define ST_DEVICE_NOT_FOUND  0x03
>   uint32_t status; /* op_code status (for reply) */
>  
>  } __attribute__((packed));
> @@ -93,7 +95,6 @@ struct op_export_request {
>  } __attribute__((packed));
>  
>  struct op_export_reply {
> - int returncode;
>  } __attribute__((packed));
>  
>  
> @@ -115,7 +116,6 @@ struct op_unexport_request {
>  } __attribute__((packed));
>  
>  struct op_unexport_reply {
> - int returncode;
>  } __attribute__((packed));
>  
>  #define PACK_OP_UNEXPORT_REQUEST(pack, request)  do {\
> 

-- 
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 1/1] usbip: auto retry for concurrent attach

Hi,

On 12/26/2016 05:53 AM, Nobuo Iwata wrote:
> This patch adds recovery from false busy state on concurrent attach 
> operation.
> 
> The procedure of attach operation is as below.
> 
> 1) Find an unused port in /sys/devices/platform/vhci_hcd/status. 
> (userspace)
> 
> 2) Request attach found port to driver through 
> /sys/devices/platform/vhci_hcd/attach. (userspace)
> 
> 3) Lock table, reserve requested port and unlock table. (vhci driver)
> 
> Attaching more than one remote devices concurrently, same unused port 
> number will be found in step-1. Then one request will succeed and 
> others will fail even though there are some unused ports. 
> 
> It is possible to avoid this fail by introdocing userspace exclusive 
> control. But it's exaggerated for this special condition. The locking 
> itself is done in driver.
> 
> With this patch, driver returns EBUSY when requested port has already 
> been used. In this case, attach command retries from step-1: finding 
> another unused port. If there's no unused port, the attach operation 
> will fail. Othrwise it retries automatically using new free port.
> 
> Currunt userspace src/usbip_attach.c:import_device() doesn't use the 
> errno.
> 
> vhci-hcd's interface (only errno) is changed as following.
> 
> Current   errno   New errno   Condition
> EINVALsame as leftspecified port numbre is in invalid 
> range
> EAGAINsame as leftplatform_get_drvdata() failed
> EINVALsame as leftspecified socket fd is not valid
> EINVALEBUSY   specified port status is not free
> 
> Signed-off-by: Nobuo Iwata 
> ---
>  drivers/usb/usbip/vhci_sysfs.c |  8 ++--
>  tools/usb/usbip/src/usbip_attach.c | 28 +++-
>  2 files changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
> index b96e5b1..5d4be4b 100644
> --- a/drivers/usb/usbip/vhci_sysfs.c
> +++ b/drivers/usb/usbip/vhci_sysfs.c
> @@ -214,7 +214,7 @@ static ssize_t store_detach(struct device *dev, struct 
> device_attribute *attr,
>  
>   ret = vhci_port_disconnect(hcd_to_vhci(hcd), rhport);
>   if (ret < 0)
> - return -EINVAL;
> + return ret;
>  
>   usbip_dbg_vhci_sysfs("Leave\n");
>  
> @@ -314,7 +314,11 @@ static ssize_t store_attach(struct device *dev, struct 
> device_attribute *attr,
>   sockfd_put(socket);
>  
>   dev_err(dev, "port %d already used\n", rhport);
> - return -EINVAL;
> + /*
> +  * Will be retried from userspace
> +  * if there's another free port.
> +  */
> + return -EBUSY;
>   }
>  
>   dev_info(dev, "pdev(%u) rhport(%u) sockfd(%d)\n",
> diff --git a/tools/usb/usbip/src/usbip_attach.c 
> b/tools/usb/usbip/src/usbip_attach.c
> index 70a6b50..695b2e5 100644
> --- a/tools/usb/usbip/src/usbip_attach.c
> +++ b/tools/usb/usbip/src/usbip_attach.c
> @@ -101,20 +101,22 @@ static int import_device(int sockfd, struct 
> usbip_usb_device *udev)
>   return -1;
>   }
>  
> - port = usbip_vhci_get_free_port();
> - if (port < 0) {
> - err("no free port");
> - usbip_vhci_driver_close();
> - return -1;
> - }
> + do {
> + port = usbip_vhci_get_free_port();
> + if (port < 0) {
> + err("no free port");
> + usbip_vhci_driver_close();
> + return -1;
> + }
>  
> - rc = usbip_vhci_attach_device(port, sockfd, udev->busnum,
> -   udev->devnum, udev->speed);
> - if (rc < 0) {
> - err("import device");
> - usbip_vhci_driver_close();
> - return -1;
> - }
> + rc = usbip_vhci_attach_device(port, sockfd, udev->busnum,
> +   udev->devnum, udev->speed);
> +     if (rc < 0 && errno != EBUSY) {
> + err("import device");
> + usbip_vhci_driver_close();
> + return -1;
> + }

copy-paste in error path. Could you please fix this to use goto as only
for this single function usbip_vhci_driver_close() is called in 3
different places.

> + } while (rc < 0);
>  
>   usbip_vhci_driver_close();
>  
> 

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: [PATCH v13 05/10] usbip: exporting devices: modifications to daemon

2017-01-04 Thread Krzysztof Opasiak



On 01/04/2017 12:48 AM, fx IWATA NOBUO wrote:
> Dear Krzysztof,
> 
> I posted new version and it's improved thanks to your review.

I've seen it. It's on my TODO list. I'll get to this in next week
(hopefully in the beginning of the week).

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: [PATCH] usbip: vudc: check for NULL before use

2016-12-21 Thread Krzysztof Opasiak

Hi,

On 12/21/2016 03:38 PM, Shuah Khan wrote:
> Hi Sudip,
> 
> On Wed, Dec 21, 2016 at 6:33 AM, Sudip Mukherjee
>  wrote:
>> On Tue, Dec 20, 2016 at 07:31:44AM -0700, Shuah Khan wrote:
>>> On 12/18/2016 03:44 PM, Sudip Mukherjee wrote:
>>>> to_vep() is doing a container_of() on _ep. It is better to do the NULL
>>>> check first and then use it.
>>>>
>>>> Signed-off-by: Sudip Mukherjee 
>>>> ---
>>>>  drivers/usb/usbip/vudc_dev.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
>>>> index 968471b..32ea604 100644
>>>> --- a/drivers/usb/usbip/vudc_dev.c
>>>> +++ b/drivers/usb/usbip/vudc_dev.c
>>>> @@ -388,10 +388,10 @@ static int vep_dequeue(struct usb_ep *_ep, struct 
>>>> usb_request *_req)
>>>> unsigned long flags;
>>>> int ret = 0;
>>>>
>>>> -   ep = to_vep(_ep);
>>>> if (!_ep)
>>>> return -EINVAL;
>>>
>>> Hmm. Linus's latest checks _ep and _req. Are you sure you are working
>>> with the latest tree?
>>
>> I checked with next-20161221 and its still there.
> 
> This is for  vep_dequeue() - Are you sure both linux-next and Linus's tree 
> show
> the following:
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/usbip/vudc_dev.c
> 
> static int vep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> {
> struct vep *ep;
> struct vrequest *req;
> struct vudc *udc;
> struct vrequest *lst;
> unsigned long flags;
> int ret = -EINVAL;
> 
> if (!_ep || !_req)
> return ret;
> 
> ep = to_vep(_ep);
> req = to_vrequest(_req);
> udc = req->udc;
> 
> if (!udc->driver)
> return -ESHUTDOWN;
> 
> There are other routines in this file that don't check null. I am confused.
> 

generally this file contains implementation of gadget/endpoint
callbacks. Those methods will be called by udc-core with values passed
from USB gadget (usually directly from function).

I check other udc in kernel and there is no agreement if we should check
in those callbacks if our params are NULL or not.

I have also run through udc-core implementation and generally it doesn't
check if params are NULL or not and just dereference some of them and
pass them to our callbacks.

I think that the best option is just to ask Felipe (USB gadget
maintainer) if we should check this or not.


@Felipe
Should each UDC check if values passed to gadget/endpoint callbacks is
not NULL or just simply dereference them?


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

[PATCH v2 1/2] usb: gadget: ether: Add \n to each attribute of ethernet functions

2016-12-21 Thread Krzysztof Opasiak

Generally in SysFS and ConfigFS files are new line terminated.
Also most of USB functions adds a trailing newline to each attribute.
Let's follow this convention also in ethernet functions.

Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/function/u_ether.c  | 24 
 drivers/usb/gadget/function/u_ether_configfs.h |  2 +-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/u_ether.c 
b/drivers/usb/gadget/function/u_ether.c
index 5d1bd13a56c1..8a50d1ccab5d 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -918,9 +918,16 @@ EXPORT_SYMBOL_GPL(gether_set_dev_addr);
 int gether_get_dev_addr(struct net_device *net, char *dev_addr, int len)
 {
struct eth_dev *dev;
+   int ret;
 
dev = netdev_priv(net);
-   return get_ether_addr_str(dev->dev_mac, dev_addr, len);
+   ret = get_ether_addr_str(dev->dev_mac, dev_addr, len);
+   if (ret + 1 < len) {
+   dev_addr[ret++] = '\n';
+   dev_addr[ret] = '\0';
+   }
+
+   return ret;
 }
 EXPORT_SYMBOL_GPL(gether_get_dev_addr);
 
@@ -940,9 +947,16 @@ EXPORT_SYMBOL_GPL(gether_set_host_addr);
 int gether_get_host_addr(struct net_device *net, char *host_addr, int len)
 {
struct eth_dev *dev;
+   int ret;
 
dev = netdev_priv(net);
-   return get_ether_addr_str(dev->host_mac, host_addr, len);
+   ret = get_ether_addr_str(dev->host_mac, host_addr, len);
+   if (ret + 1 < len) {
+   host_addr[ret++] = '\n';
+   host_addr[ret] = '\0';
+   }
+
+   return ret;
 }
 EXPORT_SYMBOL_GPL(gether_get_host_addr);
 
@@ -989,10 +1003,12 @@ EXPORT_SYMBOL_GPL(gether_get_qmult);
 
 int gether_get_ifname(struct net_device *net, char *name, int len)
 {
+   int ret;
+
rtnl_lock();
-   strlcpy(name, netdev_name(net), len);
+   ret = snprintf(name, len, "%s\n", netdev_name(net));
rtnl_unlock();
-   return strlen(name);
+   return ret < len ? ret : len;
 }
 EXPORT_SYMBOL_GPL(gether_get_ifname);
 
diff --git a/drivers/usb/gadget/function/u_ether_configfs.h 
b/drivers/usb/gadget/function/u_ether_configfs.h
index 4f47289fcf7c..c71133de17e7 100644
--- a/drivers/usb/gadget/function/u_ether_configfs.h
+++ b/drivers/usb/gadget/function/u_ether_configfs.h
@@ -108,7 +108,7 @@
mutex_lock(&opts->lock);\
qmult = gether_get_qmult(opts->net);\
mutex_unlock(&opts->lock);  \
-   return sprintf(page, "%d", qmult);  \
+   return sprintf(page, "%d\n", qmult);\
}   \
\
static ssize_t _f_##_opts_qmult_store(struct config_item *item, \
-- 
2.9.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

[PATCH v2 2/2] usb: gadget: printer: Remove pnp_string static buffer

2016-12-21 Thread Krzysztof Opasiak

pnp string is usually much shorter than 1k so let's stop wasting 1k of
memory for its buffer and make it dynamically alocated.
This also removes 1k len limitation for pnp_string and
adds a new line after string content if required.

Signed-off-by: Krzysztof Opasiak 
---
Changes since v1:
- assignt true instead of statement with no effect (reported by kbuild)
---
 drivers/usb/gadget/function/f_printer.c | 57 +
 drivers/usb/gadget/function/u_printer.h |  5 ++-
 drivers/usb/gadget/legacy/printer.c | 28 +---
 3 files changed, 62 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/gadget/function/f_printer.c 
b/drivers/usb/gadget/function/f_printer.c
index 0de36cda6e41..36fe181e72c5 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -49,7 +49,6 @@
 
 #include "u_printer.h"
 
-#define PNP_STRING_LEN 1024
 #define PRINTER_MINORS 4
 #define GET_DEVICE_ID  0
 #define GET_PORT_STATUS1
@@ -907,8 +906,7 @@ static bool gprinter_req_match(struct usb_function *f,
switch (ctrl->bRequest) {
case GET_DEVICE_ID:
w_index >>= 8;
-   if (w_length <= PNP_STRING_LEN &&
-   (USB_DIR_IN & ctrl->bRequestType))
+   if (USB_DIR_IN & ctrl->bRequestType)
break;
return false;
case GET_PORT_STATUS:
@@ -937,6 +935,7 @@ static int printer_func_setup(struct usb_function *f,
struct printer_dev *dev = func_to_printer(f);
struct usb_composite_dev *cdev = f->config->cdev;
struct usb_request  *req = cdev->req;
+   u8  *buf = req->buf;
int value = -EOPNOTSUPP;
u16 wIndex = le16_to_cpu(ctrl->wIndex);
u16 wValue = le16_to_cpu(ctrl->wValue);
@@ -953,10 +952,16 @@ static int printer_func_setup(struct usb_function *f,
if ((wIndex>>8) != dev->interface)
break;
 
-   value = (dev->pnp_string[0] << 8) | dev->pnp_string[1];
-   memcpy(req->buf, dev->pnp_string, value);
+   if (!dev->pnp_string) {
+   value = 0;
+   break;
+   }
+   value = strlen(dev->pnp_string);
+   buf[0] = (value >> 8) & 0xFF;
+   buf[1] = value & 0xFF;
+   memcpy(buf + 2, dev->pnp_string, value);
DBG(dev, "1284 PNP String: %x %s\n", value,
-   &dev->pnp_string[2]);
+   dev->pnp_string);
break;
 
case GET_PORT_STATUS: /* Get Port Status */
@@ -964,7 +969,7 @@ static int printer_func_setup(struct usb_function *f,
if (wIndex != dev->interface)
break;
 
-   *(u8 *)req->buf = dev->printer_status;
+   buf[0] = dev->printer_status;
value = min_t(u16, wLength, 1);
break;
 
@@ -1157,10 +1162,21 @@ static ssize_t f_printer_opts_pnp_string_show(struct 
config_item *item,
  char *page)
 {
struct f_printer_opts *opts = to_f_printer_opts(item);
-   int result;
+   int result = 0;
 
mutex_lock(&opts->lock);
-   result = strlcpy(page, opts->pnp_string + 2, PNP_STRING_LEN - 2);
+   if (!opts->pnp_string)
+   goto unlock;
+
+   result = strlcpy(page, opts->pnp_string, PAGE_SIZE);
+   if (result >= PAGE_SIZE) {
+   result = PAGE_SIZE;
+   } else if (page[result - 1] != '\n' && result + 1 < PAGE_SIZE) {
+   page[result++] = '\n';
+   page[result] = '\0';
+   }
+
+unlock:
mutex_unlock(&opts->lock);
 
return result;
@@ -1170,13 +1186,24 @@ static ssize_t f_printer_opts_pnp_string_store(struct 
config_item *item,
   const char *page, size_t len)
 {
struct f_printer_opts *opts = to_f_printer_opts(item);
-   int result, l;
+   char *new_pnp;
+   int result;
 
mutex_lock(&opts->lock);
-   result = strlcpy(opts->pnp_string + 2, page, PNP_STRING_LEN - 2);
-   l = strlen(opts->pnp_string + 2) + 2;
-   opts->pnp_string[0] = (l >> 8) & 0xFF;
-   opts->pnp_string[1] = l & 0xFF;
+
+   new_pnp = kstrndup(page, len, GFP_KERNEL);
+   if (!new_pnp) {
+   result = -ENOMEM;
+   g

[PATCH v2] tools: usb: usbip: Update README

Update README file:
- remove outdated parts
- clarify terminology and general structure
- add some description of vUDC

Signed-off-by: Krzysztof Opasiak 
---
Changes since v1:
- Add server and client definition
- rephrase modules description
---
 tools/usb/usbip/README | 57 +-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/README b/tools/usb/usbip/README
index 831f49fea3ce..5eb2b6c7722b 100644
--- a/tools/usb/usbip/README
+++ b/tools/usb/usbip/README
@@ -4,10 +4,33 @@
 # Copyright (C) 2011 matt mooney 
 #   2005-2008 Takahiro Hirofuchi
 
+[Overview]
+USB/IP protocol allows to pass USB device from server to client over the
+network. Server is a machine which provides (shares) a USB device. Client is
+a machine which uses USB device provided by server over the network.
+The USB device may be either physical device connected to a server or
+software entity created on a server using USB gadget subsystem.
+Whole project consists of four parts:
+
+- usbip-vhci
+A client side kernel module which provides a virtual USB Host 
Controller
+and allows to import a USB device from a remote machine.
+
+- usbip-host (stub driver)
+A server side module which provides a USB device driver which can be
+bound to a physical USB device to make it exportable.
+
+- usbip-vudc
+A server side module which provides a virtual USB Device Controller 
and allows
+to export a USB device created using USB Gadget Subsystem.
+
+- usbip-utils
+A set of userspace tools used to handle connection and management.
+Used on both sides.
 
 [Requirements]
 - USB/IP device drivers
-   Found in the staging directory of the Linux kernel.
+Found in the drivers/usb/usbip/ directory of the Linux kernel tree.
 
 - libudev >= 2.0
libudev library
@@ -36,6 +59,10 @@
 
 
 [Usage]
+On a server side there are two entities which can be shared.
+First of them is physical usb device connected to the machine.
+To make it available below steps should be executed:
+
 server:# (Physically attach your USB device.)
 
 server:# insmod usbip-core.ko
@@ -52,6 +79,30 @@
- The USB device 1-2 is now exportable to other hosts!
- Use `usbip unbind --busid 1-2' to stop exporting the device.
 
+Second of shareable entities is USB Gadget created using USB Gadget Subsystem
+on a server machine. To make it available below steps should be executed:
+
+server:# (Create your USB gadget)
+- Currently the most preferable way of creating a new USB gadget
+  is ConfigFS Composite Gadget. Please refer to its documentation
+  for details.
+- See vudc_server_example.sh for a short example of USB gadget creation
+
+server:# insmod usbip-core.ko
+server:# insmod usbip-vudc.ko
+- To create more than one instance of vudc use num module param
+
+server:# (Bind gadget to one of available vudc)
+- Assign your new gadget to USB/IP UDC
+- Using ConfigFS interface you may do this simply by:
+server:# cd /sys/kernel/config/usb_gadget/
+server:# echo "usbip-vudc.0" > UDC
+
+server:# usbipd -D --device
+- Start usbip daemon.
+
+To attach new device to client machine below commands should be used:
+
 client:# insmod usbip-core.ko
 client:# insmod vhci-hcd.ko
 
@@ -60,6 +111,8 @@
 
 client:# usbip attach --remote  --busid 1-2
- Connect the remote USB device.
+   - When using vudc on a server side busid is really vudc instance name.
+ For example: usbip-vudc.0
 
 client:# usbip port
- Show virtual port status.
@@ -192,6 +245,8 @@ Detach the imported device:
- http://usbip.wiki.sourceforge.net/how-to-debug-usbip
 - usbip-host.ko must be bound to the target device.
- See /proc/bus/usb/devices and find "Driver=..." lines of the device.
+- Target USB gadget must be bound to vudc
+  (using USB gadget susbsys, not usbip bind command)
 - Shutdown firewall.
- usbip now uses TCP port 3240.
 - Disable SELinux.
-- 
2.9.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

[PATCH 1/2] usb: gadget: ether: Add \n to each attribute of ethernet functions

Generally in SysFS and ConfigFS files are new line terminated.
Also most of USB functions adds a trailing newline to each attribute.
Let's follow this convention also in ethernet functions.

Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/function/u_ether.c  | 24 
 drivers/usb/gadget/function/u_ether_configfs.h |  2 +-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/u_ether.c 
b/drivers/usb/gadget/function/u_ether.c
index 5d1bd13a56c1..8a50d1ccab5d 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -918,9 +918,16 @@ EXPORT_SYMBOL_GPL(gether_set_dev_addr);
 int gether_get_dev_addr(struct net_device *net, char *dev_addr, int len)
 {
struct eth_dev *dev;
+   int ret;
 
dev = netdev_priv(net);
-   return get_ether_addr_str(dev->dev_mac, dev_addr, len);
+   ret = get_ether_addr_str(dev->dev_mac, dev_addr, len);
+   if (ret + 1 < len) {
+   dev_addr[ret++] = '\n';
+   dev_addr[ret] = '\0';
+   }
+
+   return ret;
 }
 EXPORT_SYMBOL_GPL(gether_get_dev_addr);
 
@@ -940,9 +947,16 @@ EXPORT_SYMBOL_GPL(gether_set_host_addr);
 int gether_get_host_addr(struct net_device *net, char *host_addr, int len)
 {
struct eth_dev *dev;
+   int ret;
 
dev = netdev_priv(net);
-   return get_ether_addr_str(dev->host_mac, host_addr, len);
+   ret = get_ether_addr_str(dev->host_mac, host_addr, len);
+   if (ret + 1 < len) {
+   host_addr[ret++] = '\n';
+   host_addr[ret] = '\0';
+   }
+
+   return ret;
 }
 EXPORT_SYMBOL_GPL(gether_get_host_addr);
 
@@ -989,10 +1003,12 @@ EXPORT_SYMBOL_GPL(gether_get_qmult);
 
 int gether_get_ifname(struct net_device *net, char *name, int len)
 {
+   int ret;
+
rtnl_lock();
-   strlcpy(name, netdev_name(net), len);
+   ret = snprintf(name, len, "%s\n", netdev_name(net));
rtnl_unlock();
-   return strlen(name);
+   return ret < len ? ret : len;
 }
 EXPORT_SYMBOL_GPL(gether_get_ifname);
 
diff --git a/drivers/usb/gadget/function/u_ether_configfs.h 
b/drivers/usb/gadget/function/u_ether_configfs.h
index 4f47289fcf7c..c71133de17e7 100644
--- a/drivers/usb/gadget/function/u_ether_configfs.h
+++ b/drivers/usb/gadget/function/u_ether_configfs.h
@@ -108,7 +108,7 @@
mutex_lock(&opts->lock);\
qmult = gether_get_qmult(opts->net);\
mutex_unlock(&opts->lock);  \
-   return sprintf(page, "%d", qmult);  \
+   return sprintf(page, "%d\n", qmult);\
}   \
\
static ssize_t _f_##_opts_qmult_store(struct config_item *item, \
-- 
2.9.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

[PATCH] usb: gadget: composite: Test get_alt() presence instead of set_alt()

By convention (according to doc) if function does not provide
get_alt() callback composite framework should assume that it has only
altsetting 0 and should respond with error if host tries to set
other one.

After commit dd4dff8b035f ("USB: composite: Fix bug: should test
set_alt function pointer before use it")
we started checking set_alt() callback instead of get_alt().
This check is useless as we check if set_alt() is set inside
usb_add_function() and fail if it's NULL.

Let's fix this check and move comment about why we check the get
method instead of set a little bit closer to prevent future false
fixes.

Fixes: dd4dff8b035f ("USB: composite: Fix bug: should test set_alt function 
pointer before use it")
Cc: stable 
Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/gadget/composite.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 32176f779861..bd88b724d085 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1689,9 +1689,7 @@ composite_setup(struct usb_gadget *gadget, const struct 
usb_ctrlrequest *ctrl)
value = min(w_length, (u16) 1);
break;
 
-   /* function drivers must handle get/set altsetting; if there's
-* no get() method, we know only altsetting zero works.
-*/
+   /* function drivers must handle get/set altsetting */
case USB_REQ_SET_INTERFACE:
if (ctrl->bRequestType != USB_RECIP_INTERFACE)
goto unknown;
@@ -1700,7 +1698,13 @@ composite_setup(struct usb_gadget *gadget, const struct 
usb_ctrlrequest *ctrl)
f = cdev->config->interface[intf];
if (!f)
break;
-   if (w_value && !f->set_alt)
+
+   /*
+* If there's no get_alt() method, we know only altsetting zero
+* works. There is no need to check if set_alt() is not NULL
+* as we check this in usb_add_function().
+*/
+   if (w_value && !f->get_alt)
break;
value = f->set_alt(f, w_index, w_value);
if (value == USB_GADGET_DELAYED_STATUS) {
-- 
2.9.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

[PATCH 2/2] usb: gadget: printer: Remove pnp_string static buffer