On Fri, Oct 15, 2010 at 04:26:29PM -0700, Sarah Sharp wrote:
> On Sat, Oct 16, 2010 at 12:23:01AM +0200, Marcin Slusarz wrote:
> > On Fri, Oct 15, 2010 at 02:59:15PM -0700, Sarah Sharp wrote:
> > > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > > index c08928a..93d3bf4 100644
> > > --- a/drivers/usb/host/xhci.h
> > > +++ b/drivers/usb/host/xhci.h
> > > @@ -1405,8 +1405,15 @@ int xhci_init(struct usb_hcd *hcd);
> > > int xhci_run(struct usb_hcd *hcd);
> > > void xhci_stop(struct usb_hcd *hcd);
> > > void xhci_shutdown(struct usb_hcd *hcd);
> > > +
> > > +#ifdef CONFIG_PM
> > > int xhci_suspend(struct xhci_hcd *xhci);
> > > int xhci_resume(struct xhci_hcd *xhci, bool hibernated);
> > > +#else
> > > +#define xhci_suspend NULL
> > > +#define xhci_resume NULL
> > > +#endif
> > > +
> >
> > "static inline int xhci_suspend(struct xhci_hcd *) {}"
> > has the same effect but saves types
>
> That doesn't have the same effect. Since those functions are only used
> as function pointers, the original patch compiles to less code when
> CONFIG_PM=n.
xhci_suspend is not called through function pointer. xhci_bus_suspend is.
> Also, the original version will cause an oops if those
> functions are ever called when CONFIG_PM=n, which would indicate a bug
> in the callee (something we would rather catch anyway). This is the
> style that other USB host controller drivers use, like EHCI.
>
> I don't understand what you mean by "saves types". Is there something
> I've missed?
Depending on CONFIG_PM xhci_suspend is a function or a constant.
If someone in the future would decide to call this function directly, without
CONFIG_PM, he would fail with "error: called object â0uâ is not a function".
So, strictly speaking, this patch is not losing type informations. It's just wrong.
Sometimes people are tempted to do something like this:
#define xhci_suspend(X) (-EINVAL)
or when "function" does not return anything:
#define void_fun(X) do {} while (0)
or even worse:
#define void fun(X) {}
And this loses type information.
Without CONFIG_PM sloppy developer could call this "function" like this:
struct whatever *p = xhci_suspend("foo bar");
and it would compile fine. Defining it as
static inline int xhci_suspend(struct xhci_hcd *) { return -EINVAL; }
would prevent it.
But in this particular case I think the cleanest solutions is to just ifdef
everything out, without fallbacks. There's only one caller of xhci_suspend
and it's wrongly guarded by CONFIG_PM. Let's just fix.
---
From: Marcin Slusarz <marcin.slusarz@xxxxxxxxx>
Subject: [PATCH] usb/xhci: fix !CONFIG_PM compile/link errors
drivers/usb/host/xhci.c:675: error: implicit declaration of function 'usb_root_hub_lost_power'
ERROR: "xhci_bus_resume" [drivers/usb/host/xhci-hcd.ko] undefined!
ERROR: "xhci_bus_suspend" [drivers/usb/host/xhci-hcd.ko] undefined!
Signed-off-by: Marcin Slusarz <marcin.slusarz@xxxxxxxxx>
---
drivers/usb/host/xhci-hub.c | 5 -----
drivers/usb/host/xhci-pci.c | 2 ++
drivers/usb/host/xhci.c | 2 ++
drivers/usb/host/xhci.h | 4 ++++
4 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 7f2f63c..ac63141 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -742,9 +742,4 @@ int xhci_bus_resume(struct usb_hcd *hcd)
return 0;
}
-#else
-
-#define xhci_bus_suspend NULL
-#define xhci_bus_resume NULL
-
#endif
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index bb668a8..d10b45d 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -189,8 +189,10 @@ static const struct hc_driver xhci_pci_hc_driver = {
/* Root hub support */
.hub_control = xhci_hub_control,
.hub_status_data = xhci_hub_status_data,
+#ifdef CONFIG_PM
.bus_suspend = xhci_bus_suspend,
.bus_resume = xhci_bus_resume,
+#endif
};
/*-------------------------------------------------------------------------*/
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 33d0034..c9fc85a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -551,6 +551,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
xhci_readl(xhci, &xhci->op_regs->status));
}
+#ifdef CONFIG_PM
static void xhci_save_registers(struct xhci_hcd *xhci)
{
xhci->s3.command = xhci_readl(xhci, &xhci->op_regs->command);
@@ -760,6 +761,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
spin_unlock_irq(&xhci->lock);
return 0;
}
+#endif
/*-------------------------------------------------------------------------*/
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index c08928a..6f6ee54 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1405,8 +1405,10 @@ int xhci_init(struct usb_hcd *hcd);
int xhci_run(struct usb_hcd *hcd);
void xhci_stop(struct usb_hcd *hcd);
void xhci_shutdown(struct usb_hcd *hcd);
+#ifdef CONFIG_PM
int xhci_suspend(struct xhci_hcd *xhci);
int xhci_resume(struct xhci_hcd *xhci, bool hibernated);
+#endif
int xhci_get_frame(struct usb_hcd *hcd);
irqreturn_t xhci_irq(struct usb_hcd *hcd);
irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd);
@@ -1481,8 +1483,10 @@ void xhci_ring_ep_doorbell(struct xhci_hcd *xhci, unsigned int slot_id,
int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
char *buf, u16 wLength);
int xhci_hub_status_data(struct usb_hcd *hcd, char *buf);
+#ifdef CONFIG_PM
int xhci_bus_suspend(struct usb_hcd *hcd);
int xhci_bus_resume(struct usb_hcd *hcd);
+#endif
u32 xhci_port_state_to_neutral(u32 state);
int xhci_find_slot_id_by_port(struct xhci_hcd *xhci, u16 port);
void xhci_ring_device(struct xhci_hcd *xhci, int slot_id);
--
1.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
