| Message ID | 20230111113018.459199-2-gregkh@linuxfoundation.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,01/16] of: device: make of_device_uevent_modalias() take a const device * | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 98 lines checked |
| robh/patch-applied | fail | build log |
On Wed, Jan 11, 2023 at 5:30 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > of_device_uevent_modalias() does not modify the device pointer passed to > it, so mark it constant. In order to properly do this, a number of > busses need to have a modalias function added as they were attempting to > just point to of_device_uevent_modalias instead of their bus-specific > modalias function. This is fine except if the prototype for a bus and > device type modalias function diverges and then problems could happen. To > prevent all of that, just wrap the call to of_device_uevent_modalias() > directly for each bus and device type individually. Why not just put the wrapper function in the DT code instead of making 4 copies of it? Rob
On Wed, Jan 11, 2023 at 08:54:04AM -0600, Rob Herring wrote: > On Wed, Jan 11, 2023 at 5:30 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > of_device_uevent_modalias() does not modify the device pointer passed to > > it, so mark it constant. In order to properly do this, a number of > > busses need to have a modalias function added as they were attempting to > > just point to of_device_uevent_modalias instead of their bus-specific > > modalias function. This is fine except if the prototype for a bus and > > device type modalias function diverges and then problems could happen. To > > prevent all of that, just wrap the call to of_device_uevent_modalias() > > directly for each bus and device type individually. > > Why not just put the wrapper function in the DT code instead of making > 4 copies of it? I could, if you think that it would be better there instead of in each individual bus (like all of the other bus callbacks). This way each bus "owns" their implementation :) thanks, greg k-h
On 11/01/2023 17:26, Greg Kroah-Hartman wrote: > On Wed, Jan 11, 2023 at 08:54:04AM -0600, Rob Herring wrote: >> On Wed, Jan 11, 2023 at 5:30 AM Greg Kroah-Hartman >> <gregkh@linuxfoundation.org> wrote: >>> >>> of_device_uevent_modalias() does not modify the device pointer passed to >>> it, so mark it constant. In order to properly do this, a number of >>> busses need to have a modalias function added as they were attempting to >>> just point to of_device_uevent_modalias instead of their bus-specific >>> modalias function. This is fine except if the prototype for a bus and >>> device type modalias function diverges and then problems could happen. To >>> prevent all of that, just wrap the call to of_device_uevent_modalias() >>> directly for each bus and device type individually. >> >> Why not just put the wrapper function in the DT code instead of making >> 4 copies of it? > > I could, if you think that it would be better there instead of in each > individual bus (like all of the other bus callbacks). This way each bus > "owns" their implementation :) I'd vote for the generic wrapper instead of 4 similar wrapper. In the end, if of_device_uevent_modalias (or the bus callback) interface changes again for whatever reasons, there will be just a single place to fix rather than fixing 4 (or more) bus drivers.
On Wed, Jan 11, 2023 at 08:54:04AM -0600, Rob Herring wrote: > On Wed, Jan 11, 2023 at 5:30 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > of_device_uevent_modalias() does not modify the device pointer passed to > > it, so mark it constant. In order to properly do this, a number of > > busses need to have a modalias function added as they were attempting to > > just point to of_device_uevent_modalias instead of their bus-specific > > modalias function. This is fine except if the prototype for a bus and > > device type modalias function diverges and then problems could happen. To > > prevent all of that, just wrap the call to of_device_uevent_modalias() > > directly for each bus and device type individually. > > Why not just put the wrapper function in the DT code instead of making > 4 copies of it? Ok, I looked at doing this today, but in the end, making "4" copies of this is simpler overall. To do it your way would require a "const" version of the function be added to the core, and then convert these 4 busses to use that, and then when the real function is converted to be const, move all of these functions back over to use that again. Lots of churn, and then in the end, we still have the mismatch of a the same function callback being used in two different types of callbacks (one a bus, one a class). This way we separate them to make things much more obvious and self-contained. So I'll keep this as-is for now, thanks. greg k-h
diff --git a/arch/powerpc/platforms/pseries/ibmebus.c b/arch/powerpc/platforms/pseries/ibmebus.c index a870cada7acd..58b798a0e879 100644 --- a/arch/powerpc/platforms/pseries/ibmebus.c +++ b/arch/powerpc/platforms/pseries/ibmebus.c @@ -426,9 +426,14 @@ static struct attribute *ibmebus_bus_device_attrs[] = { }; ATTRIBUTE_GROUPS(ibmebus_bus_device); +static int ibmebus_bus_modalias(struct device *dev, struct kobj_uevent_env *env) +{ + return of_device_uevent_modalias(dev, env); +} + struct bus_type ibmebus_bus_type = { .name = "ibmebus", - .uevent = of_device_uevent_modalias, + .uevent = ibmebus_bus_modalias, .bus_groups = ibmbus_bus_groups, .match = ibmebus_bus_bus_match, .probe = ibmebus_bus_device_probe, diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c index 3aa91aed3bf7..a180af11e034 100644 --- a/drivers/bus/sunxi-rsb.c +++ b/drivers/bus/sunxi-rsb.c @@ -172,12 +172,17 @@ static void sunxi_rsb_device_remove(struct device *dev) drv->remove(to_sunxi_rsb_device(dev)); } +static int sunxi_rsb_device_modalias(struct device *dev, struct kobj_uevent_env *env) +{ + return of_device_uevent_modalias(dev, env); +} + static struct bus_type sunxi_rsb_bus = { .name = RSB_CTRL_NAME, .match = sunxi_rsb_device_match, .probe = sunxi_rsb_device_probe, .remove = sunxi_rsb_device_remove, - .uevent = of_device_uevent_modalias, + .uevent = sunxi_rsb_device_modalias, }; static void sunxi_rsb_dev_release(struct device *dev) diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c index f5741b45ca07..e31a0261c53e 100644 --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c @@ -161,9 +161,14 @@ static void dp_aux_ep_dev_release(struct device *dev) kfree(aux_ep_with_data); } +static int dp_aux_ep_dev_modalias(struct device *dev, struct kobj_uevent_env *env) +{ + return of_device_uevent_modalias(dev, env); +} + static struct device_type dp_aux_device_type_type = { .groups = dp_aux_ep_dev_groups, - .uevent = of_device_uevent_modalias, + .uevent = dp_aux_ep_dev_modalias, .release = dp_aux_ep_dev_release, }; diff --git a/drivers/macintosh/macio_asic.c b/drivers/macintosh/macio_asic.c index 3bc1f374e657..7f2b0107c733 100644 --- a/drivers/macintosh/macio_asic.c +++ b/drivers/macintosh/macio_asic.c @@ -128,12 +128,17 @@ static int macio_device_resume(struct device * dev) return 0; } +static int macio_device_modalias(struct device *dev, struct kobj_uevent_env *env) +{ + return of_device_uevent_modalias(dev, env); +} + extern const struct attribute_group *macio_dev_groups[]; struct bus_type macio_bus_type = { .name = "macio", .match = macio_bus_match, - .uevent = of_device_uevent_modalias, + .uevent = macio_device_modalias, .probe = macio_device_probe, .remove = macio_device_remove, .shutdown = macio_device_shutdown, diff --git a/drivers/of/device.c b/drivers/of/device.c index c674a13c3055..dda51b7ce597 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -248,7 +248,7 @@ const void *of_device_get_match_data(const struct device *dev) } EXPORT_SYMBOL(of_device_get_match_data); -static ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len) +static ssize_t of_device_get_modalias(const struct device *dev, char *str, ssize_t len) { const char *compat; char *c; @@ -372,7 +372,7 @@ void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env) mutex_unlock(&of_mutex); } -int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env) +int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env) { int sl; diff --git a/include/linux/of_device.h b/include/linux/of_device.h index ab7d557d541d..f4b57614979d 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -36,7 +36,7 @@ extern ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len); extern int of_device_request_module(struct device *dev); extern void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env); -extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env); +extern int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env); static inline struct device_node *of_cpu_device_node_get(int cpu) { @@ -83,7 +83,7 @@ static inline int of_device_request_module(struct device *dev) return -ENODEV; } -static inline int of_device_uevent_modalias(struct device *dev, +static inline int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env) { return -ENODEV;
of_device_uevent_modalias() does not modify the device pointer passed to it, so mark it constant. In order to properly do this, a number of busses need to have a modalias function added as they were attempting to just point to of_device_uevent_modalias instead of their bus-specific modalias function. This is fine except if the prototype for a bus and device type modalias function diverges and then problems could happen. To prevent all of that, just wrap the call to of_device_uevent_modalias() directly for each bus and device type individually. Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Nicholas Piggin <npiggin@gmail.com> Cc: Christophe Leroy <christophe.leroy@csgroup.eu> Cc: Chen-Yu Tsai <wens@csie.org> Cc: Jernej Skrabec <jernej.skrabec@gmail.com> Cc: Samuel Holland <samuel@sholland.org> Cc: David Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Frank Rowand <frowand.list@gmail.com> Cc: Liang He <windhl@126.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Cc: Douglas Anderson <dianders@chromium.org> Cc: Lyude Paul <lyude@redhat.com> Cc: Corentin Labbe <clabbe@baylibre.com> Cc: Zou Wei <zou_wei@huawei.com> Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-kernel@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-sunxi@lists.linux.dev Cc: dri-devel@lists.freedesktop.org Cc: devicetree@vger.kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- arch/powerpc/platforms/pseries/ibmebus.c | 7 ++++++- drivers/bus/sunxi-rsb.c | 7 ++++++- drivers/gpu/drm/display/drm_dp_aux_bus.c | 7 ++++++- drivers/macintosh/macio_asic.c | 7 ++++++- drivers/of/device.c | 4 ++-- include/linux/of_device.h | 4 ++-- 6 files changed, 28 insertions(+), 8 deletions(-)