[PATCH] usb: host: ehci: allow ehci_* symbols to be unused

[PATCH] usb: host: ehci: allow ehci_* symbols to be unused

[Felipe Balbi]

not all platforms will use all of those ehci_*
symbols on their hc_driver structure. Sometimes
we might need to provide a modified version of
a certain method or not provide it at all, as is
the case with OMAPs which don't support port handoff
feature.

Whenever we compile a kernel for an OMAP board with
EHCI enabled, we get compile warnings:

drivers/usb/host/ehci-hub.c:1079: warning: 'ehci_relinquish_port' \
	defined but not used
drivers/usb/host/ehci-hub.c:1088: warning: 'ehci_port_handed_over' \
	defined but not used

In order to cleanup those warnings, we're adding
__maybe_unused annotation to those functions. We're
also trying to cope with other platforms which might
have similar issues with different methods by adding
the same annotation to all ehci_* functions.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---

Alan, what do you think about this patch ? It fixes our warnings
and tries to cope with other possible such cases.

 drivers/usb/host/ehci-hub.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 77bbb23..d50571b 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -107,7 +107,7 @@ static void ehci_handover_companion_ports(struct ehci_hcd *ehci)
 	ehci->owned_ports = 0;
 }
 
-static int ehci_port_change(struct ehci_hcd *ehci)
+static int __maybe_unused ehci_port_change(struct ehci_hcd *ehci)
 {
 	int i = HCS_N_PORTS(ehci->hcs_params);
 
@@ -201,7 +201,7 @@ static __maybe_unused void ehci_adjust_port_wakeup_flags(struct ehci_hcd *ehci,
 	spin_unlock_irqrestore(&ehci->lock, flags);
 }
 
-static int ehci_bus_suspend (struct usb_hcd *hcd)
+static int __maybe_unused ehci_bus_suspend (struct usb_hcd *hcd)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);
 	int			port;
@@ -335,7 +335,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
 
 
 /* caller has locked the root hub, and should reset/reinit on error */
-static int ehci_bus_resume (struct usb_hcd *hcd)
+static int __maybe_unused ehci_bus_resume (struct usb_hcd *hcd)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);
 	u32			temp;
@@ -550,7 +550,7 @@ static int check_reset_complete (
 
 /* build "status change" packet (one or two bytes) from HC registers */
 
-static int
+static int __maybe_unused
 ehci_hub_status_data (struct usb_hcd *hcd, char *buf)
 {
 	struct ehci_hcd	*ehci = hcd_to_ehci (hcd);
@@ -623,7 +623,7 @@ ehci_hub_status_data (struct usb_hcd *hcd, char *buf)
 
 /*-------------------------------------------------------------------------*/
 
-static void
+static void __maybe_unused
 ehci_hub_descriptor (
 	struct ehci_hcd			*ehci,
 	struct usb_hub_descriptor	*desc
@@ -658,7 +658,7 @@ ehci_hub_descriptor (
 
 /*-------------------------------------------------------------------------*/
 
-static int ehci_hub_control (
+static int __maybe_unused ehci_hub_control (
 	struct usb_hcd	*hcd,
 	u16		typeReq,
 	u16		wValue,
@@ -1076,7 +1076,8 @@ error_exit:
 	return retval;
 }
 
-static void ehci_relinquish_port(struct usb_hcd *hcd, int portnum)
+static void __maybe_unused ehci_relinquish_port(struct usb_hcd *hcd,
+		int portnum)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
 
@@ -1085,7 +1086,8 @@ static void ehci_relinquish_port(struct usb_hcd *hcd, int portnum)
 	set_owner(ehci, --portnum, PORT_OWNER);
 }
 
-static int ehci_port_handed_over(struct usb_hcd *hcd, int portnum)
+static int __maybe_unused ehci_port_handed_over(struct usb_hcd *hcd,
+		int portnum)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
 	u32 __iomem		*reg;
-- 
1.7.9

Re: [PATCH] usb: host: ehci: allow ehci_* symbols to be unused

[Alan Stern]

On Tue, 14 Feb 2012, Felipe Balbi wrote:

> not all platforms will use all of those ehci_*
> symbols on their hc_driver structure. Sometimes
> we might need to provide a modified version of
> a certain method or not provide it at all, as is
> the case with OMAPs which don't support port handoff
> feature.
> 
> Whenever we compile a kernel for an OMAP board with
> EHCI enabled, we get compile warnings:
> 
> drivers/usb/host/ehci-hub.c:1079: warning: 'ehci_relinquish_port' \
> 	defined but not used
> drivers/usb/host/ehci-hub.c:1088: warning: 'ehci_port_handed_over' \
> 	defined but not used
> 
> In order to cleanup those warnings, we're adding
> __maybe_unused annotation to those functions. We're
> also trying to cope with other platforms which might
> have similar issues with different methods by adding
> the same annotation to all ehci_* functions.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> 
> Alan, what do you think about this patch ? It fixes our warnings
> and tries to cope with other possible such cases.

Some of this makes sense: ehci_port_change, ehci_relinquish_port, 
and ehci_port_handed_over are fine.

The others are questionable.  What reason could there be for not 
implementing ehci_bus_suspend and ehci_bus_resume?  Neither of those 
should involve much platform-specific work, if any.

ehci_hub_status_data, ehci_hub_descriptor, and ehci_hub_control are 
even more puzzling.  These are things that _have_ to be implemented for 
the driver to work at all.  Platform code shouldn't replace them; it 
most it should call them as subroutines and override the results when 
necessary.

Alan Stern

Re: [PATCH] usb: host: ehci: allow ehci_* symbols to be unused

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2669 bytes --]

On Tue, Feb 14, 2012 at 10:06:47AM -0500, Alan Stern wrote:
> On Tue, 14 Feb 2012, Felipe Balbi wrote:
> 
> > not all platforms will use all of those ehci_*
> > symbols on their hc_driver structure. Sometimes
> > we might need to provide a modified version of
> > a certain method or not provide it at all, as is
> > the case with OMAPs which don't support port handoff
> > feature.
> > 
> > Whenever we compile a kernel for an OMAP board with
> > EHCI enabled, we get compile warnings:
> > 
> > drivers/usb/host/ehci-hub.c:1079: warning: 'ehci_relinquish_port' \
> > 	defined but not used
> > drivers/usb/host/ehci-hub.c:1088: warning: 'ehci_port_handed_over' \
> > 	defined but not used
> > 
> > In order to cleanup those warnings, we're adding
> > __maybe_unused annotation to those functions. We're
> > also trying to cope with other platforms which might
> > have similar issues with different methods by adding
> > the same annotation to all ehci_* functions.
> > 
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> > 
> > Alan, what do you think about this patch ? It fixes our warnings
> > and tries to cope with other possible such cases.
> 
> Some of this makes sense: ehci_port_change, ehci_relinquish_port, 
> and ehci_port_handed_over are fine.
> 
> The others are questionable.  What reason could there be for not 
> implementing ehci_bus_suspend and ehci_bus_resume?  Neither of those 
> should involve much platform-specific work, if any.

you never know what kind of silicon erratas might come up, right ? :-)

> ehci_hub_status_data, ehci_hub_descriptor, and ehci_hub_control are 
> even more puzzling.  These are things that _have_ to be implemented for 
> the driver to work at all.  Platform code shouldn't replace them; it 
> most it should call them as subroutines and override the results when 
> necessary.

There's no doubt it needs to be implemented, but the fact is that in
some situations we might need to implement it differently because of
whatever reason.

See for example that I have a pending workaround to implement for
OMAP3 EHCI where we need to reparent a clock before handling
SetFeature(port_suspend) so I would need to reimplement
echi_hub_control() in order not to expose that detail to core ehci.

Because clock reparenting is something so ARCH-specific it makes no
sense to come up with a "quirk" flag for that.

Granted, the way I had envisioned, I would reimplement the particular
request that needs special handling and call the generic
ehci_hub_control() otherwise, but still. Like I said, you never know
what kind of erratas could pop up.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: host: ehci: allow ehci_* symbols to be unused

[Alan Stern]

On Tue, 14 Feb 2012, Felipe Balbi wrote:

> > The others are questionable.  What reason could there be for not 
> > implementing ehci_bus_suspend and ehci_bus_resume?  Neither of those 
> > should involve much platform-specific work, if any.
> 
> you never know what kind of silicon erratas might come up, right ? :-)

Well, when they come up we can annotate those routines.

> > ehci_hub_status_data, ehci_hub_descriptor, and ehci_hub_control are 
> > even more puzzling.  These are things that _have_ to be implemented for 
> > the driver to work at all.  Platform code shouldn't replace them; it 
> > most it should call them as subroutines and override the results when 
> > necessary.
> 
> There's no doubt it needs to be implemented, but the fact is that in
> some situations we might need to implement it differently because of
> whatever reason.
> 
> See for example that I have a pending workaround to implement for
> OMAP3 EHCI where we need to reparent a clock before handling
> SetFeature(port_suspend) so I would need to reimplement
> echi_hub_control() in order not to expose that detail to core ehci.
> 
> Because clock reparenting is something so ARCH-specific it makes no
> sense to come up with a "quirk" flag for that.
> 
> Granted, the way I had envisioned, I would reimplement the particular
> request that needs special handling and call the generic
> ehci_hub_control() otherwise, but still. Like I said, you never know
> what kind of erratas could pop up.

It's hard to imagine what sort of errata could require you not to call
the generic routine.  Besides, it's a big routine, and duplicating it
in platform-specific code would be a big waste.

For now I think you should annotate just the three routines that really
need it.  This is analogous to patches that add new features with no
usages in the kernel; we just don't do such things.

Alan Stern

[PATCH v2] usb: host: ehci: allow ehci_* symbols to be unused

[Felipe Balbi]

not all platforms will use all of those ehci_*
symbols on their hc_driver structure. Sometimes
we might need to provide a modified version of
a certain method or not provide it at all, as is
the case with OMAPs which don't support port handoff
feature.

Whenever we compile a kernel for an OMAP board with
EHCI enabled, we get compile warnings:

drivers/usb/host/ehci-hub.c:1079: warning: 'ehci_relinquish_port' \
	defined but not used
drivers/usb/host/ehci-hub.c:1088: warning: 'ehci_port_handed_over' \
	defined but not used

In order to cleanup those warnings, we're adding
__maybe_unused annotation to those functions.

Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---

Changes from v1:
	- only add __maybe_used to relinquish_port, port_handed_over
		and port_change

 drivers/usb/host/ehci-hub.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 77bbb23..01011dd 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -107,7 +107,7 @@ static void ehci_handover_companion_ports(struct ehci_hcd *ehci)
 	ehci->owned_ports = 0;
 }
 
-static int ehci_port_change(struct ehci_hcd *ehci)
+static int __maybe_unused ehci_port_change(struct ehci_hcd *ehci)
 {
 	int i = HCS_N_PORTS(ehci->hcs_params);
 
@@ -1076,7 +1076,8 @@ error_exit:
 	return retval;
 }
 
-static void ehci_relinquish_port(struct usb_hcd *hcd, int portnum)
+static void __maybe_unused ehci_relinquish_port(struct usb_hcd *hcd,
+		int portnum)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
 
@@ -1085,7 +1086,8 @@ static void ehci_relinquish_port(struct usb_hcd *hcd, int portnum)
 	set_owner(ehci, --portnum, PORT_OWNER);
 }
 
-static int ehci_port_handed_over(struct usb_hcd *hcd, int portnum)
+static int __maybe_unused ehci_port_handed_over(struct usb_hcd *hcd,
+		int portnum)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
 	u32 __iomem		*reg;
-- 
1.7.9

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: host: ehci: allow ehci_* symbols to be unused

[Alan Stern]

On Wed, 15 Feb 2012, Felipe Balbi wrote:

> not all platforms will use all of those ehci_*
> symbols on their hc_driver structure. Sometimes
> we might need to provide a modified version of
> a certain method or not provide it at all, as is
> the case with OMAPs which don't support port handoff
> feature.
> 
> Whenever we compile a kernel for an OMAP board with
> EHCI enabled, we get compile warnings:
> 
> drivers/usb/host/ehci-hub.c:1079: warning: 'ehci_relinquish_port' \
> 	defined but not used
> drivers/usb/host/ehci-hub.c:1088: warning: 'ehci_port_handed_over' \
> 	defined but not used
> 
> In order to cleanup those warnings, we're adding
> __maybe_unused annotation to those functions.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> 
> Changes from v1:
> 	- only add __maybe_used to relinquish_port, port_handed_over
> 		and port_change

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH v2] usb: host: ehci: allow ehci_* symbols to be unused

[Govindraj]

On Wed, Feb 15, 2012 at 1:04 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> not all platforms will use all of those ehci_*
> symbols on their hc_driver structure. Sometimes
> we might need to provide a modified version of
> a certain method or not provide it at all, as is
> the case with OMAPs which don't support port handoff
> feature.
>
> Whenever we compile a kernel for an OMAP board with
> EHCI enabled, we get compile warnings:
>
> drivers/usb/host/ehci-hub.c:1079: warning: 'ehci_relinquish_port' \
>        defined but not used
> drivers/usb/host/ehci-hub.c:1088: warning: 'ehci_port_handed_over' \
>        defined but not used
>
> In order to cleanup those warnings, we're adding
> __maybe_unused annotation to those functions.
>
> Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> ---
>
> Changes from v1:
>        - only add __maybe_used to relinquish_port, port_handed_over
>                and port_change
>
>  drivers/usb/host/ehci-hub.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> index 77bbb23..01011dd 100644
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
> @@ -107,7 +107,7 @@ static void ehci_handover_companion_ports(struct ehci_hcd *ehci)
>        ehci->owned_ports = 0;
>  }
>
> -static int ehci_port_change(struct ehci_hcd *ehci)
> +static int __maybe_unused ehci_port_change(struct ehci_hcd *ehci)

should be __maybe_used ? and in rest of the patch

>  {
>        int i = HCS_N_PORTS(ehci->hcs_params);
>
> @@ -1076,7 +1076,8 @@ error_exit:
>        return retval;
>  }
>
> -static void ehci_relinquish_port(struct usb_hcd *hcd, int portnum)
> +static void __maybe_unused ehci_relinquish_port(struct usb_hcd *hcd,
> +               int portnum)
>  {
>        struct ehci_hcd         *ehci = hcd_to_ehci(hcd);
>
> @@ -1085,7 +1086,8 @@ static void ehci_relinquish_port(struct usb_hcd *hcd, int portnum)
>        set_owner(ehci, --portnum, PORT_OWNER);
>  }
>
> -static int ehci_port_handed_over(struct usb_hcd *hcd, int portnum)
> +static int __maybe_unused ehci_port_handed_over(struct usb_hcd *hcd,
> +               int portnum)
>  {
>        struct ehci_hcd         *ehci = hcd_to_ehci(hcd);
>        u32 __iomem             *reg;
> --
> 1.7.9
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: host: ehci: allow ehci_* symbols to be unused

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2821 bytes --]

Hi,

On Wed, Feb 15, 2012 at 09:27:35PM +0530, Govindraj wrote:
> On Wed, Feb 15, 2012 at 1:04 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> > not all platforms will use all of those ehci_*
> > symbols on their hc_driver structure. Sometimes
> > we might need to provide a modified version of
> > a certain method or not provide it at all, as is
> > the case with OMAPs which don't support port handoff
> > feature.
> >
> > Whenever we compile a kernel for an OMAP board with
> > EHCI enabled, we get compile warnings:
> >
> > drivers/usb/host/ehci-hub.c:1079: warning: 'ehci_relinquish_port' \
> >        defined but not used
> > drivers/usb/host/ehci-hub.c:1088: warning: 'ehci_port_handed_over' \
> >        defined but not used
> >
> > In order to cleanup those warnings, we're adding
> > __maybe_unused annotation to those functions.
> >
> > Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> > ---
> >
> > Changes from v1:
> >        - only add __maybe_used to relinquish_port, port_handed_over
> >                and port_change
> >
> >  drivers/usb/host/ehci-hub.c |    8 +++++---
> >  1 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> > index 77bbb23..01011dd 100644
> > --- a/drivers/usb/host/ehci-hub.c
> > +++ b/drivers/usb/host/ehci-hub.c
> > @@ -107,7 +107,7 @@ static void ehci_handover_companion_ports(struct ehci_hcd *ehci)
> >        ehci->owned_ports = 0;
> >  }
> >
> > -static int ehci_port_change(struct ehci_hcd *ehci)
> > +static int __maybe_unused ehci_port_change(struct ehci_hcd *ehci)
> 
> should be __maybe_used ? and in rest of the patch

why ? That isn't even defined anywhere. Take a look at
<linux/compiler-gcc.h>

| /*
|  * From the GCC manual:
|  *
|  * Many functions have no effects except the return value and their
|  * return value depends only on the parameters and/or global
|  * variables.  Such a function can be subject to common subexpression
|  * elimination and loop optimization just as an arithmetic operator
|  * would be.
|  * [...]
|  */
| #define __pure				__attribute__((pure))
| #define __aligned(x)			__attribute__((aligned(x)))
| #define __printf(a,b)			__attribute__((format(printf,a,b)))
| #define  noinline			__attribute__((noinline))
| #define __attribute_const__		__attribute__((__const__))
| #define __maybe_unused			__attribute__((unused))
| #define __always_unused			__attribute__((unused))

did you try to search for a definition of __maybe_used ? It doesn't
exist, try:

$ git grep -e __maybe_used include/

on a git kernel tree. Besides, if we had a __maybe_used flag, it
wouldn't survive very long as semantically they would be essentially the
same.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]