On Fri, May 30, 2014 at 7:47 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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.
usb_unlocked_disable_lpm() is defined in hub_pm.c and called from
hub.c driver.c and message.c. I was moving fast and just centralized
all declarations hub/hub_pm.c defined routines in hub.h. Would you
rather have one-off definitions in usb.h?
>> --- 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?
It seems I randomly made the opposite choice here, but the same
question still stands about one-off hub/hub_pm.c declarations in
usb.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.
Ok.
--
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
