On Thu, 29 May 2014, Dan Williams wrote:
> > Duplicating code like this is a little awkward. My preference would be
> > to move both usb_remote_wakeup() and hub_handle_remote_wakeup() out
> > from underneath the #if CONFIG_PM section.
> >
>
> Hmm, why not go all the way and nuke CONFIG_PM and CONFIG_PM_RUNTIME
> completely from hub.c?
>
> Is this too ugly? This moves all code in hub.c that is ifdef guarded by
> CONFIG_PM to hub_pm.c. It ends up needing to mark 11 routines in hub.c
> as global so that hub_pm.c can consume them. It's not perfect as it
> still requires ifdef CONFIG_PM_RUNTIME in hub_pm.c itself, but certainly
> gets us closer to the "no ifdef in .c files" ideal.
>
> Compiles for me under CONFIG_PM=y and CONFIG_PM=n.
The general idea seems okay. A few of the details are questionable...
> --- a/drivers/usb/core/Kconfig
> +++ b/drivers/usb/core/Kconfig
> @@ -90,3 +90,6 @@ config USB_OTG_FSM
> Implements OTG Finite State Machine as specified in On-The-Go
> and Embedded Host Supplement to the USB Revision 2.0 Specification.
>
> +config USB_HUB_PM
> + def_bool y
> + depends on PM || PM_RUNTIME
PM is always set if PM_RUNTIME is. So this always ends up setting
USB_HUB_PM equal to PM, which means USB_HUB_PM isn't needed.
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -29,7 +29,7 @@
> #include <linux/usb/quirks.h>
> #include <linux/usb/hcd.h>
>
> -#include "usb.h"
> +#include "hub.h"
Why on earth would something like this be needed? You didn't remove
anything from usb.h.
All the changes in this patch should be internal to the hub driver.
Other .c files should not be involved.
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -152,3 +152,82 @@ static inline int hub_port_debounce_be_stable(struct usb_hub *hub,
> return hub_port_debounce(hub, port1, false);
> }
>
> +enum hub_quiescing_type {
> + HUB_DISCONNECT, HUB_PRE_RESET, HUB_SUSPEND
> +};
> +
> +enum hub_activation_type {
> + HUB_INIT, HUB_INIT2, HUB_INIT3, /* INITs must come first */
> + HUB_POST_RESET, HUB_RESUME, HUB_RESET_RESUME,
> +};
> +
> +/* support routines consumed by hub_pm.c */
> +extern int usb_port_is_power_on(struct usb_hub *hub, unsigned portstatus);
> +extern void usb_lock_port(struct usb_port *port_dev)
> + __acquires(&port_dev->status_lock);
> +extern void usb_unlock_port(struct usb_port *port_dev)
> + __releases(&port_dev->status_lock);
> +extern int hub_set_port_link_state(struct usb_hub *hub, int port1,
> + unsigned int link_status);
> +extern int usb_set_port_feature(struct usb_device *hdev, int port1, int feature);
> +extern int usb_reset_and_verify_device(struct usb_device *udev);
> +extern int hub_port_status(struct usb_hub *hub, int port1,
> + u16 *status, u16 *change);
> +extern void hub_port_logical_disconnect(struct usb_hub *hub, int port1);
> +extern int hub_port_disable(struct usb_hub *hub, int port1, int set_state);
> +extern void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type);
> +void hub_activate(struct usb_hub *hub, enum hub_activation_type type);
> +
> +#ifdef CONFIG_PM
> +extern int usb_disable_lpm(struct usb_device *udev);
> +extern void usb_enable_lpm(struct usb_device *udev);
> +extern int usb_unlocked_disable_lpm(struct usb_device *udev);
> +extern void usb_unlocked_enable_lpm(struct usb_device *udev);
> +extern int usb_disable_ltm(struct usb_device *udev);
> +extern void usb_enable_ltm(struct usb_device *udev);
> +extern int hub_suspend(struct usb_interface *intf, pm_message_t msg);
> +extern int hub_resume(struct usb_interface *intf);
> +extern int hub_reset_resume(struct usb_interface *intf);
Since these functions are now going into the global namespace, their
names should all begin with "usb_". You need to rename all the
"hub_*" functions to "usb_hub_*".
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -16,7 +16,7 @@
> #include <linux/usb/hcd.h> /* for usbcore internals */
> #include <asm/byteorder.h>
>
> -#include "usb.h"
> +#include "hub.h"
Like with driver.c, this should not be needed.
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -111,6 +111,11 @@ static inline int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
> {
> return 0;
> }
> +
> +static inline int usb_remote_wakeup(struct usb_device *dev)
> +{
> + return 0;
> +}
> #endif
It seems strange for this to be here. Why not move all the
declarations of usb_remote_wakeup into hub.h?
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -592,6 +592,17 @@ struct usb_device {
> };
> #define to_usb_device(d) container_of(d, struct usb_device, dev)
>
> +#ifdef CONFIG_PM
> +static inline void usb_set_reset_resume(struct usb_device *udev)
> +{
> + udev->reset_resume = 1;
> +}
> +#else
> +static inline void usb_set_reset_resume(struct usb_device *udev)
> +{
> +}
> +#endif
Maybe this belongs in a separate patch. It's not directly connected to
moving all the PM-related parts of hub.c into a separate file.
Alan Stern
--
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
