Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add files via upload #101

Closed
wants to merge 7 commits into from
Closed

Add files via upload #101

wants to merge 7 commits into from

Conversation

burki-de
Copy link

Hello Greg,

thank you for your description how to send patches ;-)

I added a new option -f to lsusb to be havea more flexible list format and to be able to print the serial number in hexadecimal.
Reason is, that I've several ST-Link/V2 programmer, which use binary data for serial number string :-(
Now I can switch to the hex dump and can grep the device I'm looking for ;-)

Copy link
Owner

@gregkh gregkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, see below for individual comments.

Also, the changelog text here needs a lot of work, read up on how to properly describe a commit. ANd we need a signed-off-by as is documented in the Linux kernel documentation

NEWS Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
lsusb.c Outdated Show resolved Hide resolved
lsusb.c Outdated Show resolved Hide resolved
lsusb.c Outdated Show resolved Hide resolved
lsusb.c Outdated Show resolved Hide resolved

} else {
// For text output
char *serial = get_dev_string(udev, desc->iSerialNumber);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is odd, the serial number should work to be retrieved this way as well.

I think your device is broken and does not follow the specification, as this is required to be a string and not "hex values", correct? Have you asked if the device passes the USB certification process?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read about and agree that the ST.-Link/V2 device does abuse serial
number field. String should be a UTF-16 LE text.
But in reality it contains e.g. 0x06 and 0x02 as a character code.
The higher bytes are always zero.
Here my 2 devices with the modified lsusb:
5400ff006d00060072006600530053003000400002006700
4900ff007100060049007000530055004400460006008700
The device itself is working as it should with exception of the serial number.
I'm not the only one with this problem:

@@ -3829,6 +3937,13 @@ int main(int argc, char *argv[])
" -d vendor:[product]\n"
" Show only devices with the specified vendor and\n"
" product ID numbers (in hexadecimal)\n"
" -f FORMAT\n"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have two different things being added in one change. One is the format operation, which is nice to have. The second is your new ability to handle serial numbers in hex format, which is different.

Please break this up into at least 2 different changes and resubmit them as a patch series.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, right. I thought better to add some also useful functions instead of making a quick and dirty modification.
You really need to split? I hoped to show you that It's a useful feature even if normally only needed with devices, which breaks the rules of the serial number descriptor.
To be honest: If you make a release without the %x output it's useless for me :-(
It was the main reason why I made this changes.
And there are definitely devices with this bug :-(
If you really need it in two steps, I can split. Please send a note.

@burki-de
Copy link
Author

Hello Greg,

You asked me about a "signed-of-by". Read about what it is but don't find a place to add it somewhere in github and I think to put it somewhere into the source is a bad idea.
So I made a short, one line text file with my "signed-of-by" and added it.
Hope you can put it to the right place. If not it would be great when you give me
a hint how to handle.
Greetings from Germany

Burkhard

@burki-de burki-de requested a review from gregkh November 21, 2019 22:58
Copy link
Owner

@gregkh gregkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole commit is not needed.

@gregkh
Copy link
Owner

gregkh commented Nov 22, 2019

Please rebase your changes such that you have unique commits with different changelog texts that do not all have the same subject, and you remove the ones that are not needed.

As for the signed-off-by, that goes in the changelog text, look at all of the other commits in this tree for examples of how to do that (do a 'git log' on the repo).

If you clean that all up, and make a set of patches that only do one logical thing per commit, I'll be glad to review them.

@gregkh
Copy link
Owner

gregkh commented Jan 10, 2020

Closing due to slow response. Please feel free to fix up and resubmit and I will gladly review.

thanks!

@gregkh gregkh closed this Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants