From: David Brownell on
> > David, any thoughts on these versions

Still no fan, since it just duplicates
existing functionality from the composite
framework. Using the existing mechanism
seems preferable to me: you want a serial
number? provide one using the existing scheme
and just make sure it follows the rules.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Michał Nazarewicz on
On Sun, 18 Jul 2010 01:57:30 +0200, David Brownell <david-b(a)pacbell.net> wrote:
> Still no fan, since it just duplicates
> existing functionality from the composite
> framework. Using the existing mechanism
> seems preferable to me: you want a serial
> number? provide one using the existing scheme
> and just make sure it follows the rules.

Hello David,

I'm still not sure what you mean.

As a matter of fact I think you might be confusing Mass Storage Gadget
with File Storage Gadget.

The first patch in the series merely adds the initialisation for the
iSerialNumber field of the descriptor of Mass Storage Gadget (which
is a composite gadget). It in no way duplicates functionality of
the composite layer. As a matter of fact, without this patch, the
iSerialNumber module parameter won't work (not tested, just looked
at the code).

The second patch (by Yann Cantin) adds a serial module parameter to
the File Storage Gadget. FSG is not a composite gadget so the patch
does not duplicate composite functionality (well it does but it's
irrelevant since FSG cannot use composite anyway).

The third patch is irrelevant in this discussion I believe.

--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, Michał "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Brownell on

>   It in no way duplicates functionality of
> the composite layer.

Beyond the obvious "Set serial number" ...


> As a matter of fact, without
> this patch, the
> iSerialNumber module parameter won't work

So submit a bugfix for that alone, making it
work everywhere...

> (not tested, just looked at the code).

Do you know when it broke? That code has been
tested in the past, and observed to work. So if
it's not working now, someone broke it and that
should just be fixed.



> The second patch (by Yann Cantin) adds a serial module
> parameter to
> the File Storage Gadget.  FSG is not a composite
> gadget

OK, so it should add a module param with the same
name as used elsewhere ... makes sense to me. If
Alan Acks that patch, go for it.

- Dave


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Michał Nazarewicz on
On Mon, 19 Jul 2010 12:08:36 +0200, David Brownell <david-b(a)pacbell.net> wrote:
>> It in no way duplicates functionality of the composite layer.

> Beyond the obvious "Set serial number" ...

In that case, should all the composite gadgets stop setting
the iManufacturer, iProduct and iSerialNumber? My understanding
is that the composite module parameters are only meant as a way
to override what the module uses as default. Using your
rationale, modules should not only stop setting those fields but
also the idVendor and idProduct.

>> As a matter of fact, without this patch, the
>> iSerialNumber module parameter won't work

> So submit a bugfix for that alone, making it
> work everywhere...

iSerialNumber not working is not the main issue. The main issue
is that the serial number is not set by default which makes
Windows sad (Windows uses serial number to distinguish different
devices with the same vendor and product ID). The serial number
has to be set by default without the need for user to give a
module parameter.

So even if composite.c were to be modified the code in mass
storage gadget would have to stay anyway.

> Do you know when it broke? That code has been
> tested in the past, and observed to work. So if
> it's not working now, someone broke it and that
> should just be fixed.

It never broke. It was "broken" from the beginning. Here's part of
composite.c that handles the iSerialNumber:

if (cdev->desc.iSerialNumber && iSerialNumber)
string_override(composite->strings,
cdev->desc.iSerialNumber, iSerialNumber);

As you see, the iSerialNumber module parameter is ignored unless the
gadget driver sets the iSerialNumber field of the device descriptor.
This patch makes mass storage gadget and g_multi set it. As said
above, this is orthogonal to changing the behaviour of the
iSerialNumber module parameter.


>> The second patch (by Yann Cantin) adds a serial module
>> parameter to
>> the File Storage Gadget. FSG is not a composite
>> gadget

> OK, so it should add a module param with the same
> name as used elsewhere ...

I was going to propose that but file storage gadget uses names
different from composite anyway (ie. vendor and product instead of
idVendor and idProduct) so I dunno if it's worth it. As a matter
of fact, "serial" seems to match the other names better.

> makes sense to me. I Alan Acks that patch, go for it.

I believe Alan already agreed on this patch. I'm merely updating
it to use the changes introduced by my patch to mass storage and
g_multi.

--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, Michał "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on
On Mon, 19 Jul 2010, [utf-8] Michał Nazarewicz wrote:

> >> The second patch (by Yann Cantin) adds a serial module
> >> parameter to
> >> the File Storage Gadget. FSG is not a composite
> >> gadget
>
> > OK, so it should add a module param with the same
> > name as used elsewhere ...
>
> I was going to propose that but file storage gadget uses names
> different from composite anyway (ie. vendor and product instead of
> idVendor and idProduct) so I dunno if it's worth it. As a matter
> of fact, "serial" seems to match the other names better.
>
> > makes sense to me. I Alan Acks that patch, go for it.
>
> I believe Alan already agreed on this patch. I'm merely updating
> it to use the changes introduced by my patch to mass storage and
> g_multi.

I have lost track of the current state of that patch. When you finish
updating it, please post it for my review.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/