| Message ID | 1345235499-32556-1-git-send-email-luk0104@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Łukasz Dałek, > PXA25X chips don't support alternate settings so code in ether.c > disables usage of CDC. > But only code defined between DEV_CONFIG_CDC signals that network is up. > This patch is fixing this bug by addding pxa_connect_gadget() which on > pxa25x chips signals that network is up and do nothing on any other > chips. You're getting better, it's a good sign you're learning, I'm glad to see it :-) > Signed-off-by: Łukasz Dałek <luk0104@gmail.com> > --- > drivers/usb/gadget/ether.c | 21 ++++++++++++++++++++- > 1 files changed, 20 insertions(+), 1 deletions(-) > > diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c > index d975fb6..964fe2e 100644 > --- a/drivers/usb/gadget/ether.c > +++ b/drivers/usb/gadget/ether.c > @@ -44,7 +44,12 @@ extern struct platform_data brd; > > unsigned packet_received, packet_sent; > > -#define DEV_CONFIG_CDC 1 > +#ifdef CONFIG_USB_GADGET_PXA2XX > +# undef DEV_CONFIG_CDC > +# define DEV_CONFIG_SUBSET 1 Can't we make it into a config value? Does this work for you: 1) Add: CONFIG_USB_ETH_SUBSET CONFIG_USB_ETH_CDC 2) Change this to #if !defined(...SUBSET) || !defined(...CDC) || !defined(RNDIS) #define DEV_CONFIG_CDC /* preserve default behavior */ #endif #if defined(...SUBSET) #define DEV_CONFIG_SUBSET #endif #if defined(...CDC) #define DEV_CONFIG_CDC #endif /* Note that RNDIS is already fixed */ -- FROM HERE IT GOES INTO DIFFERENT PATCH -- 3) Replace DEV_CONFIG_CDC with CONFIG_USB_ETH_CDC, remove the last #if defined above 4) DTTO for SUBSET 5) define SUBSET in your header file is it clear? > +#else > +# define DEV_CONFIG_CDC 1 > +#endif > #define GFP_ATOMIC ((gfp_t) 0) > #define GFP_KERNEL ((gfp_t) 0) > > @@ -864,7 +869,9 @@ static struct usb_gadget_strings stringtab = { > > /*======================================================================== > ====*/ static u8 control_req[USB_BUFSIZ]; > +#if defined(DEV_CONFIG_CDC) || defined(CONFIG_USB_ETH_RNDIS) > static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4))); > +#endif What trouble do you face here? Tom, this aligned(4) doesn't look correct, some ALLOC_ALIGNED or friend would help here, right ? :) > > /** > @@ -1252,6 +1259,17 @@ static void rndis_command_complete(struct usb_ep > *ep, struct usb_request *req) > > #endif /* RNDIS */ > > +#ifdef CONFIG_USB_GADGET_PXA2XX > +static inline void pxa_connect_gadget(void) > +{ > + debug("PXA connecting gadget...\n"); > + l_ethdev.network_started = 1; > + printf("USB network up!\n"); Definitelly not printf(), but is this really PXA specific or is this part of the CDC subset goo? > +} > +#else > +static inline void pxa_connect_gadget(void) { } > +#endif > + > /* > * The setup() callback implements all the ep0 functionality that's not > * handled lower down. CDC has a number of less-common features: > @@ -1352,6 +1370,7 @@ eth_setup(struct usb_gadget *gadget, const struct > usb_ctrlrequest *ctrl) if (gadget_is_pxa(gadget)) { > value = eth_set_config(dev, DEV_CONFIG_VALUE, > GFP_ATOMIC); > + pxa_connect_gadget(); > goto done_set_intf; > } Best regards, Marek Vasut
On 17.08.2012 22:48, Marek Vasut wrote: > Dear Łukasz Dałek, > >> PXA25X chips don't support alternate settings so code in ether.c >> disables usage of CDC. >> But only code defined between DEV_CONFIG_CDC signals that network is up. >> This patch is fixing this bug by addding pxa_connect_gadget() which on >> pxa25x chips signals that network is up and do nothing on any other >> chips. > You're getting better, it's a good sign you're learning, I'm glad to see it :-) It's my first big project in which I got involved so I'm ready for criticism. >> Signed-off-by: Łukasz Dałek<luk0104@gmail.com> >> --- >> drivers/usb/gadget/ether.c | 21 ++++++++++++++++++++- >> 1 files changed, 20 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c >> index d975fb6..964fe2e 100644 >> --- a/drivers/usb/gadget/ether.c >> +++ b/drivers/usb/gadget/ether.c >> @@ -44,7 +44,12 @@ extern struct platform_data brd; >> >> unsigned packet_received, packet_sent; >> >> -#define DEV_CONFIG_CDC 1 >> +#ifdef CONFIG_USB_GADGET_PXA2XX >> +# undef DEV_CONFIG_CDC >> +# define DEV_CONFIG_SUBSET 1 > Can't we make it into a config value? > > Does this work for you: > 1) Add: > CONFIG_USB_ETH_SUBSET > CONFIG_USB_ETH_CDC > > 2) Change this to > > #if !defined(...SUBSET) || !defined(...CDC) || !defined(RNDIS) > #define DEV_CONFIG_CDC /* preserve default behavior */ > #endif > > #if defined(...SUBSET) > #define DEV_CONFIG_SUBSET > #endif > > #if defined(...CDC) > #define DEV_CONFIG_CDC > #endif > > /* Note that RNDIS is already fixed */ Ok. > > -- FROM HERE IT GOES INTO DIFFERENT PATCH -- > > 3) Replace DEV_CONFIG_CDC with CONFIG_USB_ETH_CDC, remove the last #if defined > above Which one #if? That one I'd define in previous patch? > 4) DTTO for SUBSET > > 5) define SUBSET in your header file In my board's header file? >> +#else >> +# define DEV_CONFIG_CDC 1 >> +#endif >> #define GFP_ATOMIC ((gfp_t) 0) >> #define GFP_KERNEL ((gfp_t) 0) >> >> @@ -864,7 +869,9 @@ static struct usb_gadget_strings stringtab = { >> >> /*======================================================================== >> ====*/ static u8 control_req[USB_BUFSIZ]; >> +#if defined(DEV_CONFIG_CDC) || defined(CONFIG_USB_ETH_RNDIS) >> static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4))); >> +#endif > What trouble do you face here? > > Tom, this aligned(4) doesn't look correct, some ALLOC_ALIGNED or friend would > help here, right ? :) If you don't define DEV_CONFIG_CDC nor ...RNDIS and define DEV_CONFIG_SUBSET then it won't compile (because of STATUS_BYTECOUNT). >> /** >> @@ -1252,6 +1259,17 @@ static void rndis_command_complete(struct usb_ep >> *ep, struct usb_request *req) >> >> #endif /* RNDIS */ >> >> +#ifdef CONFIG_USB_GADGET_PXA2XX >> +static inline void pxa_connect_gadget(void) >> +{ >> + debug("PXA connecting gadget...\n"); >> + l_ethdev.network_started = 1; >> + printf("USB network up!\n"); > Definitelly not printf(), but is this really PXA specific or is this part of the > CDC subset goo? I've took pattern from code in this file. What should I use instead of printf? CDC subset disable some code for PXA which signalise that network was started (l_ethdev... = 1) so this is quick-fix for this bug. Łukasz Dałek
Dear Łukasz Dałek, > On 17.08.2012 22:48, Marek Vasut wrote: > > Dear Łukasz Dałek, > > > >> PXA25X chips don't support alternate settings so code in ether.c > >> disables usage of CDC. > >> But only code defined between DEV_CONFIG_CDC signals that network is up. > >> This patch is fixing this bug by addding pxa_connect_gadget() which on > >> pxa25x chips signals that network is up and do nothing on any other > >> chips. > > > > You're getting better, it's a good sign you're learning, I'm glad to see > > it :-) > > It's my first big project in which I got involved so I'm ready for > criticism. > > >> Signed-off-by: Łukasz Dałek<luk0104@gmail.com> > >> --- > >> > >> drivers/usb/gadget/ether.c | 21 ++++++++++++++++++++- > >> 1 files changed, 20 insertions(+), 1 deletions(-) > >> > >> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c > >> index d975fb6..964fe2e 100644 > >> --- a/drivers/usb/gadget/ether.c > >> +++ b/drivers/usb/gadget/ether.c > >> @@ -44,7 +44,12 @@ extern struct platform_data brd; > >> > >> unsigned packet_received, packet_sent; > >> > >> -#define DEV_CONFIG_CDC 1 > >> +#ifdef CONFIG_USB_GADGET_PXA2XX > >> +# undef DEV_CONFIG_CDC > >> +# define DEV_CONFIG_SUBSET 1 > > > > Can't we make it into a config value? > > > > Does this work for you: > > 1) Add: > > CONFIG_USB_ETH_SUBSET > > CONFIG_USB_ETH_CDC > > > > 2) Change this to > > > > #if !defined(...SUBSET) || !defined(...CDC) || !defined(RNDIS) > > #define DEV_CONFIG_CDC /* preserve default behavior */ > > #endif > > > > #if defined(...SUBSET) > > #define DEV_CONFIG_SUBSET > > #endif > > > > #if defined(...CDC) > > #define DEV_CONFIG_CDC > > #endif > > > > /* Note that RNDIS is already fixed */ > > Ok. > > > -- FROM HERE IT GOES INTO DIFFERENT PATCH -- > > > > 3) Replace DEV_CONFIG_CDC with CONFIG_USB_ETH_CDC, remove the last #if > > defined above > > Which one #if? That one I'd define in previous patch? Yes, continuous rework so you avoid breakage and keep this bisectable. > > 4) DTTO for SUBSET > > > > 5) define SUBSET in your header file > > In my board's header file? Yes > >> +#else > >> +# define DEV_CONFIG_CDC 1 > >> +#endif > >> > >> #define GFP_ATOMIC ((gfp_t) 0) > >> #define GFP_KERNEL ((gfp_t) 0) > >> > >> @@ -864,7 +869,9 @@ static struct usb_gadget_strings stringtab = { > >> > >> /*==================================================================== > >> ==== > >> > >> ====*/ static u8 control_req[USB_BUFSIZ]; > >> +#if defined(DEV_CONFIG_CDC) || defined(CONFIG_USB_ETH_RNDIS) > >> > >> static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4))); > >> > >> +#endif > > > > What trouble do you face here? > > > > Tom, this aligned(4) doesn't look correct, some ALLOC_ALIGNED or friend > > would help here, right ? :) > > If you don't define DEV_CONFIG_CDC nor ...RNDIS and define > DEV_CONFIG_SUBSET then it won't compile (because of STATUS_BYTECOUNT). Fix STATUS_BYTECOUNT then. Why is status_req global at all? > >> /** > >> > >> @@ -1252,6 +1259,17 @@ static void rndis_command_complete(struct usb_ep > >> *ep, struct usb_request *req) > >> > >> #endif /* RNDIS */ > >> > >> +#ifdef CONFIG_USB_GADGET_PXA2XX > >> +static inline void pxa_connect_gadget(void) > >> +{ > >> + debug("PXA connecting gadget...\n"); > >> + l_ethdev.network_started = 1; > >> + printf("USB network up!\n"); > > > > Definitelly not printf(), but is this really PXA specific or is this part > > of the CDC subset goo? > > I've took pattern from code in this file. What should I use instead of > printf? debug() ? I guess we can wrap that function directly into the code anyway and drop all the debug output in it altogether, boiling it down only to l_ethdev.network_started = 1; > CDC subset disable some code for PXA which signalise that network was > started (l_ethdev... = 1) so this is quick-fix for this bug. This is really sad :-( > Łukasz Dałek Best regards, Marek Vasut
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index d975fb6..964fe2e 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -44,7 +44,12 @@ extern struct platform_data brd; unsigned packet_received, packet_sent; -#define DEV_CONFIG_CDC 1 +#ifdef CONFIG_USB_GADGET_PXA2XX +# undef DEV_CONFIG_CDC +# define DEV_CONFIG_SUBSET 1 +#else +# define DEV_CONFIG_CDC 1 +#endif #define GFP_ATOMIC ((gfp_t) 0) #define GFP_KERNEL ((gfp_t) 0) @@ -864,7 +869,9 @@ static struct usb_gadget_strings stringtab = { /*============================================================================*/ static u8 control_req[USB_BUFSIZ]; +#if defined(DEV_CONFIG_CDC) || defined(CONFIG_USB_ETH_RNDIS) static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4))); +#endif /** @@ -1252,6 +1259,17 @@ static void rndis_command_complete(struct usb_ep *ep, struct usb_request *req) #endif /* RNDIS */ +#ifdef CONFIG_USB_GADGET_PXA2XX +static inline void pxa_connect_gadget(void) +{ + debug("PXA connecting gadget...\n"); + l_ethdev.network_started = 1; + printf("USB network up!\n"); +} +#else +static inline void pxa_connect_gadget(void) { } +#endif + /* * The setup() callback implements all the ep0 functionality that's not * handled lower down. CDC has a number of less-common features: @@ -1352,6 +1370,7 @@ eth_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) if (gadget_is_pxa(gadget)) { value = eth_set_config(dev, DEV_CONFIG_VALUE, GFP_ATOMIC); + pxa_connect_gadget(); goto done_set_intf; }
PXA25X chips don't support alternate settings so code in ether.c disables usage of CDC. But only code defined between DEV_CONFIG_CDC signals that network is up. This patch is fixing this bug by addding pxa_connect_gadget() which on pxa25x chips signals that network is up and do nothing on any other chips. Signed-off-by: Łukasz Dałek <luk0104@gmail.com> --- drivers/usb/gadget/ether.c | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-)