[PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled

[PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled

[Ray Chi]

Fix build error when CONFIG_POWER_SUPPLY is not enabled.

The build error occurs in mips (cavium_octeon_defconfig).

mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_remove':
drivers/usb/dwc3/core.c:1657: undefined reference to `power_supply_put'
mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_get_properties':
drivers/usb/dwc3/core.c:1270: undefined reference to `power_supply_get_by_name'
mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_probe':
drivers/usb/dwc3/core.c:1632: undefined reference to `power_supply_put'

Fixes: 59fa3def35de ("usb: dwc3: add a power supply for current control")
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Signed-off-by: Ray Chi <raychi@google.com>
---
 include/linux/power_supply.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 81a55e974feb..6e776be5bfa0 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -381,8 +381,14 @@ struct power_supply_battery_info {
 extern struct atomic_notifier_head power_supply_notifier;
 extern int power_supply_reg_notifier(struct notifier_block *nb);
 extern void power_supply_unreg_notifier(struct notifier_block *nb);
+#if IS_ENABLED(CONFIG_POWER_SUPPLY)
 extern struct power_supply *power_supply_get_by_name(const char *name);
 extern void power_supply_put(struct power_supply *psy);
+#else
+static inline void power_supply_put(struct power_supply *psy) {}
+static inline struct power_supply *power_supply_get_by_name(const char *name)
+{ return NULL; }
+#endif
 #ifdef CONFIG_OF
 extern struct power_supply *power_supply_get_by_phandle(struct device_node *np,
 							const char *property);
-- 
2.30.1.766.gb4fecdf3b7-goog

Re: [PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled

[Sebastian Reichel]

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

Hi,

On Mon, Mar 08, 2021 at 09:31:46PM +0800, Ray Chi wrote:
> Fix build error when CONFIG_POWER_SUPPLY is not enabled.
> 
> The build error occurs in mips (cavium_octeon_defconfig).
> 
> mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_remove':
> drivers/usb/dwc3/core.c:1657: undefined reference to `power_supply_put'
> mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_get_properties':
> drivers/usb/dwc3/core.c:1270: undefined reference to `power_supply_get_by_name'
> mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_probe':
> drivers/usb/dwc3/core.c:1632: undefined reference to `power_supply_put'
> 
> Fixes: 59fa3def35de ("usb: dwc3: add a power supply for current control")
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Ray Chi <raychi@google.com>
> ---

While I'm fine with merging this after fixing up the subject, the
original patch for dwc3 [0] looks completly incorrect to me.

First of all it uses wrong scale (power-supply uses uA, not mA),
so you are charging 1000x slower than expected. Then the patchset
introduces a new DT property to get the power-supply device, but
does not update the DT binding documentation and does not Cc the
DT binding maintainer. Next the property itself looks not very
smart to me. Usually one would use a device reference, not the
Linux device name.

Finally all existing devices solve this by registering a usb
notifier from the charger, so why are you going the other way
around? This is going to break once you want to use one of the
existing chargers with dwc3.

I suggest to drop/revert the whole patchset.

[0] https://lore.kernel.org/linux-usb/20210222115149.3606776-1-raychi@google.com/

-- Sebastian

>  include/linux/power_supply.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 81a55e974feb..6e776be5bfa0 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -381,8 +381,14 @@ struct power_supply_battery_info {
>  extern struct atomic_notifier_head power_supply_notifier;
>  extern int power_supply_reg_notifier(struct notifier_block *nb);
>  extern void power_supply_unreg_notifier(struct notifier_block *nb);
> +#if IS_ENABLED(CONFIG_POWER_SUPPLY)
>  extern struct power_supply *power_supply_get_by_name(const char *name);
>  extern void power_supply_put(struct power_supply *psy);
> +#else
> +static inline void power_supply_put(struct power_supply *psy) {}
> +static inline struct power_supply *power_supply_get_by_name(const char *name)
> +{ return NULL; }
> +#endif
>  #ifdef CONFIG_OF
>  extern struct power_supply *power_supply_get_by_phandle(struct device_node *np,
>  							const char *property);
> -- 
> 2.30.1.766.gb4fecdf3b7-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled

[Ray Chi]

Hi Sebastian,

Sorry for the late reply.

On Wed, Mar 10, 2021 at 2:58 AM Sebastian Reichel <sre@kernel.org> wrote:
>
> Hi,
>
> On Mon, Mar 08, 2021 at 09:31:46PM +0800, Ray Chi wrote:
> > Fix build error when CONFIG_POWER_SUPPLY is not enabled.
> >
> > The build error occurs in mips (cavium_octeon_defconfig).
> >
> > mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_remove':
> > drivers/usb/dwc3/core.c:1657: undefined reference to `power_supply_put'
> > mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_get_properties':
> > drivers/usb/dwc3/core.c:1270: undefined reference to `power_supply_get_by_name'
> > mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_probe':
> > drivers/usb/dwc3/core.c:1632: undefined reference to `power_supply_put'
> >
> > Fixes: 59fa3def35de ("usb: dwc3: add a power supply for current control")
> > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> > Signed-off-by: Ray Chi <raychi@google.com>
> > ---
>
> While I'm fine with merging this after fixing up the subject, the
> original patch for dwc3 [0] looks completly incorrect to me.
>
> First of all it uses wrong scale (power-supply uses uA, not mA),
> so you are charging 1000x slower than expected. Then the patchset
> introduces a new DT property to get the power-supply device, but
> does not update the DT binding documentation and does not Cc the
> DT binding maintainer.

Yes, it should use uA and send this information, and I will update a
patch to fix it and add the DT binding documentation.

> Next the property itself looks not very
> smart to me. Usually one would use a device reference, not the
> Linux device name.
>
> Finally all existing devices solve this by registering a usb
> notifier from the charger, so why are you going the other way
> around? This is going to break once you want to use one of the
> existing chargers with dwc3.

Only the USB controller will know USB state/speed so that I think it is
better to send this information from the USB side.
For example:
For USB high speed, charging current should be limited to 500 mA in
configured state.
For USB super speed, charging current should be limited to 900 mA in
configured state.

>
> I suggest to drop/revert the whole patchset.
>
> [0] https://lore.kernel.org/linux-usb/20210222115149.3606776-1-raychi@google.com/
>
> -- Sebastian
>
> >  include/linux/power_supply.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > index 81a55e974feb..6e776be5bfa0 100644
> > --- a/include/linux/power_supply.h
> > +++ b/include/linux/power_supply.h
> > @@ -381,8 +381,14 @@ struct power_supply_battery_info {
> >  extern struct atomic_notifier_head power_supply_notifier;
> >  extern int power_supply_reg_notifier(struct notifier_block *nb);
> >  extern void power_supply_unreg_notifier(struct notifier_block *nb);
> > +#if IS_ENABLED(CONFIG_POWER_SUPPLY)
> >  extern struct power_supply *power_supply_get_by_name(const char *name);
> >  extern void power_supply_put(struct power_supply *psy);
> > +#else
> > +static inline void power_supply_put(struct power_supply *psy) {}
> > +static inline struct power_supply *power_supply_get_by_name(const char *name)
> > +{ return NULL; }
> > +#endif
> >  #ifdef CONFIG_OF
> >  extern struct power_supply *power_supply_get_by_phandle(struct device_node *np,
> >                                                       const char *property);
> > --
> > 2.30.1.766.gb4fecdf3b7-goog
> >

Re: [PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled

[Sebastian Reichel]

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

Hi,

On Fri, Mar 12, 2021 at 09:57:56PM +0800, Ray Chi wrote:
> > While I'm fine with merging this after fixing up the subject, the
> > original patch for dwc3 [0] looks completly incorrect to me.
> >
> > First of all it uses wrong scale (power-supply uses uA, not mA),
> > so you are charging 1000x slower than expected. Then the patchset
> > introduces a new DT property to get the power-supply device, but
> > does not update the DT binding documentation and does not Cc the
> > DT binding maintainer.
> 
> Yes, it should use uA and send this information, and I will update a
> patch to fix it and add the DT binding documentation.

Considering your programming is off by a factor 1000 I wonder how
this patchset has been tested.

> > Next the property itself looks not very
> > smart to me. Usually one would use a device reference, not the
> > Linux device name.
> >
> > Finally all existing devices solve this by registering a usb
> > notifier from the charger, so why are you going the other way
> > around? This is going to break once you want to use one of the
> > existing chargers with dwc3.
> 
> Only the USB controller will know USB state/speed so that I think
> it is better to send this information from the USB side. For
> example: For USB high speed, charging current should be limited to
> 500 mA in configured state.  For USB super speed, charging current
> should be limited to 900 mA in configured state.

usb_register_notifier registers a callback to receive information
from the USB subsystem. Then power-supply drivers can query specific
info, e.g. call usb_phy_get_charger_current(). This is already being
done by some power-supply drivers, so using one of those
power-supply charger drivers in combination with dwc3 will now result
in potentially racy behaviour as far as I can see.

> > I suggest to drop/revert the whole patchset.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled

[Ray Chi]

Hi,

On Mon, Mar 15, 2021 at 6:35 AM Sebastian Reichel <sre@kernel.org> wrote:
>
> Hi,
>
> On Fri, Mar 12, 2021 at 09:57:56PM +0800, Ray Chi wrote:
> > > While I'm fine with merging this after fixing up the subject, the
> > > original patch for dwc3 [0] looks completly incorrect to me.
> > >
> > > First of all it uses wrong scale (power-supply uses uA, not mA),
> > > so you are charging 1000x slower than expected. Then the patchset
> > > introduces a new DT property to get the power-supply device, but
> > > does not update the DT binding documentation and does not Cc the
> > > DT binding maintainer.
> >
> > Yes, it should use uA and send this information, and I will update a
> > patch to fix it and add the DT binding documentation.
>
> Considering your programming is off by a factor 1000 I wonder how
> this patchset has been tested.

Since our corresponding charging driver also uses mA as the unit, I
don't find this problem.

>
> > > Next the property itself looks not very
> > > smart to me. Usually one would use a device reference, not the
> > > Linux device name.
> > >
> > > Finally all existing devices solve this by registering a usb
> > > notifier from the charger, so why are you going the other way
> > > around? This is going to break once you want to use one of the
> > > existing chargers with dwc3.
> >
> > Only the USB controller will know USB state/speed so that I think
> > it is better to send this information from the USB side. For
> > example: For USB high speed, charging current should be limited to
> > 500 mA in configured state.  For USB super speed, charging current
> > should be limited to 900 mA in configured state.
>
> usb_register_notifier registers a callback to receive information
> from the USB subsystem. Then power-supply drivers can query specific
> info, e.g. call usb_phy_get_charger_current(). This is already being
> done by some power-supply drivers, so using one of those
> power-supply charger drivers in combination with dwc3 will now result
> in potentially racy behaviour as far as I can see.

Since these functions are defined in driver/usb/phy/phy.c, only the
devices which support usb_phy can use them.
If the device supports generic PHY drivers, it needs an additional way
to provide the information.
BTW, when there are two or more ways to provide the information, I
think it is fine to return the result directly if one of the ways is
executed successfully.

>
> > > I suggest to drop/revert the whole patchset.
>
> -- Sebastian

Re: [PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled

[Greg KH]

On Fri, Mar 12, 2021 at 09:57:56PM +0800, Ray Chi wrote:
> Hi Sebastian,
> 
> Sorry for the late reply.
> 
> On Wed, Mar 10, 2021 at 2:58 AM Sebastian Reichel <sre@kernel.org> wrote:
> >
> > Hi,
> >
> > On Mon, Mar 08, 2021 at 09:31:46PM +0800, Ray Chi wrote:
> > > Fix build error when CONFIG_POWER_SUPPLY is not enabled.
> > >
> > > The build error occurs in mips (cavium_octeon_defconfig).
> > >
> > > mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_remove':
> > > drivers/usb/dwc3/core.c:1657: undefined reference to `power_supply_put'
> > > mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_get_properties':
> > > drivers/usb/dwc3/core.c:1270: undefined reference to `power_supply_get_by_name'
> > > mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_probe':
> > > drivers/usb/dwc3/core.c:1632: undefined reference to `power_supply_put'
> > >
> > > Fixes: 59fa3def35de ("usb: dwc3: add a power supply for current control")
> > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> > > Signed-off-by: Ray Chi <raychi@google.com>
> > > ---
> >
> > While I'm fine with merging this after fixing up the subject, the
> > original patch for dwc3 [0] looks completly incorrect to me.
> >
> > First of all it uses wrong scale (power-supply uses uA, not mA),
> > so you are charging 1000x slower than expected. Then the patchset
> > introduces a new DT property to get the power-supply device, but
> > does not update the DT binding documentation and does not Cc the
> > DT binding maintainer.
> 
> Yes, it should use uA and send this information, and I will update a
> patch to fix it and add the DT binding documentation.

So should I revert what we currently have in my usb-next tree, or do
you have a fix for this?

thanks,

greg k-h

Re: [PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled

[Ray Chi]

Hi Greg,

I will upload fixes for power supply usage in dwc3 and dt-binding
documentation for the new device tree this week.

Thanks,
Ray

On Tue, Mar 23, 2021 at 9:47 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Mar 12, 2021 at 09:57:56PM +0800, Ray Chi wrote:
> > Hi Sebastian,
> >
> > Sorry for the late reply.
> >
> > On Wed, Mar 10, 2021 at 2:58 AM Sebastian Reichel <sre@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Mar 08, 2021 at 09:31:46PM +0800, Ray Chi wrote:
> > > > Fix build error when CONFIG_POWER_SUPPLY is not enabled.
> > > >
> > > > The build error occurs in mips (cavium_octeon_defconfig).
> > > >
> > > > mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_remove':
> > > > drivers/usb/dwc3/core.c:1657: undefined reference to `power_supply_put'
> > > > mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_get_properties':
> > > > drivers/usb/dwc3/core.c:1270: undefined reference to `power_supply_get_by_name'
> > > > mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_probe':
> > > > drivers/usb/dwc3/core.c:1632: undefined reference to `power_supply_put'
> > > >
> > > > Fixes: 59fa3def35de ("usb: dwc3: add a power supply for current control")
> > > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> > > > Signed-off-by: Ray Chi <raychi@google.com>
> > > > ---
> > >
> > > While I'm fine with merging this after fixing up the subject, the
> > > original patch for dwc3 [0] looks completly incorrect to me.
> > >
> > > First of all it uses wrong scale (power-supply uses uA, not mA),
> > > so you are charging 1000x slower than expected. Then the patchset
> > > introduces a new DT property to get the power-supply device, but
> > > does not update the DT binding documentation and does not Cc the
> > > DT binding maintainer.
> >
> > Yes, it should use uA and send this information, and I will update a
> > patch to fix it and add the DT binding documentation.
>
> So should I revert what we currently have in my usb-next tree, or do
> you have a fix for this?
>
> thanks,
>
> greg k-h