| Message ID | 20201112163630.17177-2-nsaenzjulienne@suse.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Raspberry Pi PoE HAT fan support | expand |
On Thu, Nov 12, 2020 at 6:40 PM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > When unbinding the firmware device we need to make sure it has no > consumers left. Otherwise we'd leave them with a firmware handle > pointing at freed memory. > > Keep a reference count of all consumers and introduce rpi_firmware_put() > which will permit automatically decrease the reference count upon > unbinding consumer drivers. ... > /** > - * rpi_firmware_get - Get pointer to rpi_firmware structure. > * @firmware_node: Pointer to the firmware Device Tree node. > * > + * The reference to rpi_firmware has to be released with rpi_firmware_put(). > + * > * Returns NULL is the firmware device is not ready. > */ > struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node) > { > struct platform_device *pdev = of_find_device_by_node(firmware_node); > + struct rpi_firmware *fw; > > if (!pdev) > return NULL; > > - return platform_get_drvdata(pdev); > + fw = platform_get_drvdata(pdev); > + if (!fw) > + return NULL; > + > + if (!kref_get_unless_zero(&fw->consumers)) > + return NULL; Don't we have a more traditional way of doing this, i.e. try_module_get() coupled with get_device() ? > + return fw; > }
On Thu, Nov 12, 2020 at 07:52:14PM +0200, Andy Shevchenko wrote: > On Thu, Nov 12, 2020 at 6:40 PM Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > > > When unbinding the firmware device we need to make sure it has no > > consumers left. Otherwise we'd leave them with a firmware handle > > pointing at freed memory. > > > > Keep a reference count of all consumers and introduce rpi_firmware_put() > > which will permit automatically decrease the reference count upon > > unbinding consumer drivers. > > ... > > > /** > > - * rpi_firmware_get - Get pointer to rpi_firmware structure. > > * @firmware_node: Pointer to the firmware Device Tree node. > > * > > + * The reference to rpi_firmware has to be released with rpi_firmware_put(). > > + * > > * Returns NULL is the firmware device is not ready. > > */ > > struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node) > > { > > struct platform_device *pdev = of_find_device_by_node(firmware_node); > > + struct rpi_firmware *fw; > > > > if (!pdev) > > return NULL; > > > > - return platform_get_drvdata(pdev); > > + fw = platform_get_drvdata(pdev); > > + if (!fw) > > + return NULL; > > + > > + if (!kref_get_unless_zero(&fw->consumers)) > > + return NULL; > > Don't we have a more traditional way of doing this, i.e. > try_module_get() coupled with get_device() ? get_device() will make sure that device is there, but gives no assurances that device is bound to a driver, so it will not help with the racy access to firmware via platform_get_drvdata() call. Thanks.
On Thu, 2020-11-12 at 23:26 -0800, Dmitry Torokhov wrote: > On Thu, Nov 12, 2020 at 07:52:14PM +0200, Andy Shevchenko wrote: > > On Thu, Nov 12, 2020 at 6:40 PM Nicolas Saenz Julienne > > <nsaenzjulienne@suse.de> wrote: > > > > > > When unbinding the firmware device we need to make sure it has no > > > consumers left. Otherwise we'd leave them with a firmware handle > > > pointing at freed memory. > > > > > > Keep a reference count of all consumers and introduce rpi_firmware_put() > > > which will permit automatically decrease the reference count upon > > > unbinding consumer drivers. > > > > ... > > > > > /** > > > - * rpi_firmware_get - Get pointer to rpi_firmware structure. > > > * @firmware_node: Pointer to the firmware Device Tree node. > > > * > > > + * The reference to rpi_firmware has to be released with rpi_firmware_put(). > > > + * > > > * Returns NULL is the firmware device is not ready. > > > */ > > > struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node) > > > { > > > struct platform_device *pdev = of_find_device_by_node(firmware_node); > > > + struct rpi_firmware *fw; > > > > > > if (!pdev) > > > return NULL; > > > > > > - return platform_get_drvdata(pdev); > > > + fw = platform_get_drvdata(pdev); > > > + if (!fw) > > > + return NULL; > > > + > > > + if (!kref_get_unless_zero(&fw->consumers)) > > > + return NULL; > > Hi Andy, Dimitry, > > Don't we have a more traditional way of doing this, i.e. > > try_module_get() coupled with get_device() ? > > get_device() will make sure that device is there, but gives no > assurances that device is bound to a driver, so it will not help with > the racy access to firmware via platform_get_drvdata() call. I also looked at using get/put_device() just as a means for refcounting (i.e. replacing fw->consumers), but I can't make it work either. I'd need a way to hook up into one of the struct device_ktype release() functions. AFAIK it's not possible for private uses like this. IIUC the way to do this would be to bypass platform device and create a special device class/bus for RPi's firmware dependent devices (I could pretty much copy SCMI's implementation), but I fear that's overkill. So, for now I'll stick with the kref based implementation, I'll be happy to change it if you find a better solution. :) Regards, Nicolas
diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c index 2371d08bdd17..438e17074a97 100644 --- a/drivers/firmware/raspberrypi.c +++ b/drivers/firmware/raspberrypi.c @@ -7,6 +7,7 @@ */ #include <linux/dma-mapping.h> +#include <linux/kref.h> #include <linux/mailbox_client.h> #include <linux/module.h> #include <linux/of_platform.h> @@ -27,6 +28,8 @@ struct rpi_firmware { struct mbox_chan *chan; /* The property channel. */ struct completion c; u32 enabled; + + struct kref consumers; }; static DEFINE_MUTEX(transaction_lock); @@ -225,12 +228,27 @@ static void rpi_register_clk_driver(struct device *dev) -1, NULL, 0); } +static void rpi_firmware_delete(struct kref *kref) +{ + struct rpi_firmware *fw = container_of(kref, struct rpi_firmware, + consumers); + + mbox_free_channel(fw->chan); + kfree(fw); +} + +void rpi_firmware_put(struct rpi_firmware *fw) +{ + kref_put(&fw->consumers, rpi_firmware_delete); +} +EXPORT_SYMBOL_GPL(rpi_firmware_put); + static int rpi_firmware_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct rpi_firmware *fw; - fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL); + fw = kzalloc(sizeof(*fw), GFP_KERNEL); if (!fw) return -ENOMEM; @@ -247,6 +265,7 @@ static int rpi_firmware_probe(struct platform_device *pdev) } init_completion(&fw->c); + kref_init(&fw->consumers); platform_set_drvdata(pdev, fw); @@ -275,25 +294,35 @@ static int rpi_firmware_remove(struct platform_device *pdev) rpi_hwmon = NULL; platform_device_unregister(rpi_clk); rpi_clk = NULL; - mbox_free_channel(fw->chan); + + rpi_firmware_put(fw); return 0; } /** - * rpi_firmware_get - Get pointer to rpi_firmware structure. * @firmware_node: Pointer to the firmware Device Tree node. * + * The reference to rpi_firmware has to be released with rpi_firmware_put(). + * * Returns NULL is the firmware device is not ready. */ struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node) { struct platform_device *pdev = of_find_device_by_node(firmware_node); + struct rpi_firmware *fw; if (!pdev) return NULL; - return platform_get_drvdata(pdev); + fw = platform_get_drvdata(pdev); + if (!fw) + return NULL; + + if (!kref_get_unless_zero(&fw->consumers)) + return NULL; + + return fw; } EXPORT_SYMBOL_GPL(rpi_firmware_get); diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h index cc9cdbc66403..fdfef7fe40df 100644 --- a/include/soc/bcm2835/raspberrypi-firmware.h +++ b/include/soc/bcm2835/raspberrypi-firmware.h @@ -140,6 +140,7 @@ int rpi_firmware_property(struct rpi_firmware *fw, u32 tag, void *data, size_t len); int rpi_firmware_property_list(struct rpi_firmware *fw, void *data, size_t tag_size); +void rpi_firmware_put(struct rpi_firmware *fw); struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node); #else static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag, @@ -154,6 +155,7 @@ static inline int rpi_firmware_property_list(struct rpi_firmware *fw, return -ENOSYS; } +static inline void rpi_firmware_put(struct rpi_firmware *fw) { } static inline struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node) { return NULL;
When unbinding the firmware device we need to make sure it has no consumers left. Otherwise we'd leave them with a firmware handle pointing at freed memory. Keep a reference count of all consumers and introduce rpi_firmware_put() which will permit automatically decrease the reference count upon unbinding consumer drivers. Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- Changes since v3: - Use kref instead of waiting on refcount drivers/firmware/raspberrypi.c | 37 +++++++++++++++++++--- include/soc/bcm2835/raspberrypi-firmware.h | 2 ++ 2 files changed, 35 insertions(+), 4 deletions(-)