diff mbox series

[v5,1/3] xhci: hub: export symbol on xhci_hub_control

Message ID 20221027133127.27592-2-jilin@nvidia.com
State New
Headers
Series xhci: tegra: USB2 pad power controls |

Commit Message

Jim Lin Oct. 27, 2022, 1:31 p.m. UTC 
  Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module
to invoke and avoid linking error.

Signed-off-by: Jim Lin <jilin@nvidia.com>

---
v5: new change

 drivers/usb/host/xhci-hub.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Greg KH Oct. 27, 2022, 1:58 p.m. UTC | #1 
On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote:
> Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module
> to invoke and avoid linking error.

What other driver module?

There is no user here :(

confused,

greg k-h
Jim Lin Oct. 28, 2022, 5:36 a.m. UTC | #2 
On Thu, 2022-10-27 at 15:58 +0200, Greg KH wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote:
> > Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module
> > to invoke and avoid linking error.
> 
> What other driver module?
> 
> There is no user here :(
> 
> confused,
> 

In arch/arm/configs/multi_v7_defconfig
It defines
CONFIG_USB_XHCI_TEGRA=m

If I don't add EXPORT_SYMBOL_GPL on xhci_hub_control()
, I will get compile/linking error like

ERROR: modpost: "xhci_hub_control" [drivers/usb/host/xhci-tegra.ko]
undefined!

if patch
"[PATCH v5,3/3] xhci: tegra: USB2 pad power controls"

https://patchwork.kernel.org/project/linux-usb/patch/20221027133127.27592-4-jilin@nvidia.com/
is added in xhci-tegra.c to invoke xhci_hub_control()

Should I integrate this patch with [PATCH v5,3/3] as one?

--nvpublic
Greg KH Oct. 28, 2022, 6:04 a.m. UTC | #3 
On Fri, Oct 28, 2022 at 05:36:41AM +0000, Jim Lin wrote:
> On Thu, 2022-10-27 at 15:58 +0200, Greg KH wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote:
> > > Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module
> > > to invoke and avoid linking error.
> > 
> > What other driver module?
> > 
> > There is no user here :(
> > 
> > confused,
> > 
> 
> In arch/arm/configs/multi_v7_defconfig
> It defines
> CONFIG_USB_XHCI_TEGRA=m
> 
> If I don't add EXPORT_SYMBOL_GPL on xhci_hub_control()
> , I will get compile/linking error like
> 
> ERROR: modpost: "xhci_hub_control" [drivers/usb/host/xhci-tegra.ko]
> undefined!
> 
> if patch
> "[PATCH v5,3/3] xhci: tegra: USB2 pad power controls"
> 
> https://patchwork.kernel.org/project/linux-usb/patch/20221027133127.27592-4-jilin@nvidia.com/
> is added in xhci-tegra.c to invoke xhci_hub_control()
> 
> Should I integrate this patch with [PATCH v5,3/3] as one?

Yes, do not add something that is not needed for that specific commit,
otherwise it causes reviewers to be confused.

thanks,

greg k-h
Thierry Reding Oct. 28, 2022, 9:50 a.m. UTC | #4 
On Fri, Oct 28, 2022 at 08:04:54AM +0200, gregkh@linuxfoundation.org wrote:
> On Fri, Oct 28, 2022 at 05:36:41AM +0000, Jim Lin wrote:
> > On Thu, 2022-10-27 at 15:58 +0200, Greg KH wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote:
> > > > Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module
> > > > to invoke and avoid linking error.
> > > 
> > > What other driver module?
> > > 
> > > There is no user here :(
> > > 
> > > confused,
> > > 
> > 
> > In arch/arm/configs/multi_v7_defconfig
> > It defines
> > CONFIG_USB_XHCI_TEGRA=m
> > 
> > If I don't add EXPORT_SYMBOL_GPL on xhci_hub_control()
> > , I will get compile/linking error like
> > 
> > ERROR: modpost: "xhci_hub_control" [drivers/usb/host/xhci-tegra.ko]
> > undefined!
> > 
> > if patch
> > "[PATCH v5,3/3] xhci: tegra: USB2 pad power controls"
> > 
> > https://patchwork.kernel.org/project/linux-usb/patch/20221027133127.27592-4-jilin@nvidia.com/
> > is added in xhci-tegra.c to invoke xhci_hub_control()
> > 
> > Should I integrate this patch with [PATCH v5,3/3] as one?
> 
> Yes, do not add something that is not needed for that specific commit,
> otherwise it causes reviewers to be confused.

Other subsystem maintainers prefer core changes to be split from driver
changes, so this type of split is commonly encountered elsewhere.

Obviously, since this is your turf you get to make the rules. I'm just
trying to say that this kind of advice can be confusing for contributors
because when they then sent driver and code changes mixed for their next
submission, the subsystem maintainer might tell them otherwise.

Thierry
Greg KH Oct. 28, 2022, 10:37 a.m. UTC | #5 
On Fri, Oct 28, 2022 at 11:50:47AM +0200, Thierry Reding wrote:
> On Fri, Oct 28, 2022 at 08:04:54AM +0200, gregkh@linuxfoundation.org wrote:
> > On Fri, Oct 28, 2022 at 05:36:41AM +0000, Jim Lin wrote:
> > > On Thu, 2022-10-27 at 15:58 +0200, Greg KH wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote:
> > > > > Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module
> > > > > to invoke and avoid linking error.
> > > > 
> > > > What other driver module?
> > > > 
> > > > There is no user here :(
> > > > 
> > > > confused,
> > > > 
> > > 
> > > In arch/arm/configs/multi_v7_defconfig
> > > It defines
> > > CONFIG_USB_XHCI_TEGRA=m
> > > 
> > > If I don't add EXPORT_SYMBOL_GPL on xhci_hub_control()
> > > , I will get compile/linking error like
> > > 
> > > ERROR: modpost: "xhci_hub_control" [drivers/usb/host/xhci-tegra.ko]
> > > undefined!
> > > 
> > > if patch
> > > "[PATCH v5,3/3] xhci: tegra: USB2 pad power controls"
> > > 
> > > https://patchwork.kernel.org/project/linux-usb/patch/20221027133127.27592-4-jilin@nvidia.com/
> > > is added in xhci-tegra.c to invoke xhci_hub_control()
> > > 
> > > Should I integrate this patch with [PATCH v5,3/3] as one?
> > 
> > Yes, do not add something that is not needed for that specific commit,
> > otherwise it causes reviewers to be confused.
> 
> Other subsystem maintainers prefer core changes to be split from driver
> changes, so this type of split is commonly encountered elsewhere.
> 
> Obviously, since this is your turf you get to make the rules. I'm just
> trying to say that this kind of advice can be confusing for contributors
> because when they then sent driver and code changes mixed for their next
> submission, the subsystem maintainer might tell them otherwise.

Sure, but if you do split it up like this, DOCUMENT WHY THE EXPORT IS
NEEDED.  That didn't happen here so I had no idea why this was even an
issue.

And yes, I am very sensitive to this, we have had LOTS of people trying
to export xhci symbols in the past few years for no in-kernel users,
despite us constantly telling them that this is not allowed.  It
happened again, just yesterday:
	https://lore.kernel.org/r/20221027004050.4192111-1-albertccwang@google.com

And at first glance, I assumed this was much the same as there was no
description of why this was needed at all.

thanks,

greg k-h
Thierry Reding Oct. 28, 2022, 11:08 a.m. UTC | #6 
On Fri, Oct 28, 2022 at 12:37:15PM +0200, gregkh@linuxfoundation.org wrote:
> On Fri, Oct 28, 2022 at 11:50:47AM +0200, Thierry Reding wrote:
> > On Fri, Oct 28, 2022 at 08:04:54AM +0200, gregkh@linuxfoundation.org wrote:
> > > On Fri, Oct 28, 2022 at 05:36:41AM +0000, Jim Lin wrote:
> > > > On Thu, 2022-10-27 at 15:58 +0200, Greg KH wrote:
> > > > > External email: Use caution opening links or attachments
> > > > > 
> > > > > 
> > > > > On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote:
> > > > > > Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module
> > > > > > to invoke and avoid linking error.
> > > > > 
> > > > > What other driver module?
> > > > > 
> > > > > There is no user here :(
> > > > > 
> > > > > confused,
> > > > > 
> > > > 
> > > > In arch/arm/configs/multi_v7_defconfig
> > > > It defines
> > > > CONFIG_USB_XHCI_TEGRA=m
> > > > 
> > > > If I don't add EXPORT_SYMBOL_GPL on xhci_hub_control()
> > > > , I will get compile/linking error like
> > > > 
> > > > ERROR: modpost: "xhci_hub_control" [drivers/usb/host/xhci-tegra.ko]
> > > > undefined!
> > > > 
> > > > if patch
> > > > "[PATCH v5,3/3] xhci: tegra: USB2 pad power controls"
> > > > 
> > > > https://patchwork.kernel.org/project/linux-usb/patch/20221027133127.27592-4-jilin@nvidia.com/
> > > > is added in xhci-tegra.c to invoke xhci_hub_control()
> > > > 
> > > > Should I integrate this patch with [PATCH v5,3/3] as one?
> > > 
> > > Yes, do not add something that is not needed for that specific commit,
> > > otherwise it causes reviewers to be confused.
> > 
> > Other subsystem maintainers prefer core changes to be split from driver
> > changes, so this type of split is commonly encountered elsewhere.
> > 
> > Obviously, since this is your turf you get to make the rules. I'm just
> > trying to say that this kind of advice can be confusing for contributors
> > because when they then sent driver and code changes mixed for their next
> > submission, the subsystem maintainer might tell them otherwise.
> 
> Sure, but if you do split it up like this, DOCUMENT WHY THE EXPORT IS
> NEEDED.  That didn't happen here so I had no idea why this was even an
> issue.
> 
> And yes, I am very sensitive to this, we have had LOTS of people trying
> to export xhci symbols in the past few years for no in-kernel users,
> despite us constantly telling them that this is not allowed.  It
> happened again, just yesterday:
> 	https://lore.kernel.org/r/20221027004050.4192111-1-albertccwang@google.com
> 
> And at first glance, I assumed this was much the same as there was no
> description of why this was needed at all.

Agreed. I suppose this could've been spelled out more explicitly in the
cover letter or in patch 1.

Jim, please make sure to describe this dependency explicitly in v6.

Thierry
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index af946c42b6f0..4f20cdae2a89 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1604,6 +1604,7 @@  int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	spin_unlock_irqrestore(&xhci->lock, flags);
 	return retval;
 }
+EXPORT_SYMBOL_GPL(xhci_hub_control);
 
 /*
  * Returns 0 if the status hasn't changed, or the number of bytes in buf.