usb: dwc3: fix build error when POWER_SUPPLY is not enabled

PATCH In review Ray Chi 3 participants 7 messages Mar 8, 2021 – Mar 23, 2021 Power supply

lore

Share & tools

lore

Ray Chi

author

2021-03-08 13:32 UTC #0 lore raw

PATCH 1/1 usb: dwc3: fix build error when POWER_SUPPLY is not enabled

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 <[email protected]>

Signed-off-by Ray Chi <[email protected]>

---
 include/linux/power_supply.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/power_supply.h +6 -0

index 81a55e974feb..6e776be5bfa0 100644
@@ -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);

Signature · 1 line

2.30.1.766.gb4fecdf3b7-goog

Sebastian Reichel

maintainer

2021-03-09 18:58 UTC #1 lore raw

Hi,

Quoted 11 lines · 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'

Quoted 4 lines

Fixes 59fa3def35de ("usb: dwc3: add a power supply for current control")
Reported-by Naresh Kamboju <[email protected]>
Signed-off-by Ray Chi <[email protected]>
---

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/[email protected]/

-- Sebastian

Quoted 25 lines

 include/linux/power_supply.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/power_supply.h +6 -0

index 81a55e974feb..6e776be5bfa0 100644
@@ -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);

Signature · 1 line

2.30.1.766.gb4fecdf3b7-goog

Ray Chi

author

2021-03-12 13:58 UTC #2 lore raw

Hi Sebastian,

Sorry for the late reply.

Quoted 20 lines · Sebastian Reichel

Hi,
Quoted 11 lines · 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'
Quoted 4 lines
Fixes 59fa3def35de ("usb: dwc3: add a power supply for current control")
Reported-by Naresh Kamboju <[email protected]>
Signed-off-by Ray Chi <[email protected]>
---

Quoted 8 lines

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.

Quoted 8 lines

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.

Quoted 32 lines

I suggest to drop/revert the whole patchset.

[0] https://lore.kernel.org/linux-usb/[email protected]/

-- Sebastian

Quoted 25 lines

 include/linux/power_supply.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/power_supply.h +6 -1

index 81a55e974feb..6e776be5bfa0 100644
@@ -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

Sebastian Reichel

maintainer

2021-03-14 22:36 UTC #3 lore raw

Hi,

Quoted 11 lines · Ray Chi

Quoted 8 lines
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.

Quoted 9 lines

Quoted 8 lines
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.

Quoted 5 lines

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.

Quoted 1 line

Quoted 1 line
I suggest to drop/revert the whole patchset.

-- Sebastian

Ray Chi

author

2021-03-15 12:52 UTC #4 lore raw

Hi,

Quoted 13 lines · Sebastian Reichel

Hi,
Quoted 9 lines · Ray Chi
Quoted 8 lines
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 16 lines

Quoted 10 lines
Quoted 9 lines
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.
Quoted 5 lines
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.

Quoted 6 lines

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.

Quoted 4 lines

Quoted 2 lines
Quoted 2 lines
I suggest to drop/revert the whole patchset.
-- Sebastian

Greg KH

2021-03-23 13:48 UTC #5 lore raw

In reply to Ray Chi

Quoted 28 lines · Ray Chi

Hi Sebastian,
Sorry for the late reply.
Quoted 23 lines · Sebastian Reichel
Hi,
Quoted 11 lines · 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'
Quoted 4 lines
Fixes 59fa3def35de ("usb: dwc3: add a power supply for current control")
Reported-by Naresh Kamboju <[email protected]>
Signed-off-by Ray Chi <[email protected]>
---
While I'm fine with merging this after fixing up the subject, the original patch for dwc3 [0] looks completly incorrect to me.

Quoted 8 lines

Quoted 5 lines
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

Ray Chi

author

2021-03-23 14:25 UTC #6 lore raw

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

Quoted 45 lines · Greg KH

Quoted 28 lines · Ray Chi
Hi Sebastian,
Sorry for the late reply.
Quoted 23 lines · Sebastian Reichel
Hi,
Quoted 11 lines · 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'
Quoted 4 lines
Fixes 59fa3def35de ("usb: dwc3: add a power supply for current control")
Reported-by Naresh Kamboju <[email protected]>
Signed-off-by Ray Chi <[email protected]>
---
While I'm fine with merging this after fixing up the subject, the original patch for dwc3 [0] looks completly incorrect to me.
Quoted 8 lines
Quoted 5 lines
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