| Message ID | 20201123183833.18750-2-nsaenzjulienne@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Raspberry Pi PoE HAT fan support | expand |
On 11/23/2020 10:38 AM, Nicolas Saenz Julienne 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. > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> This looks fine to me, just one nit below: [snip] > /** > - * rpi_firmware_get - Get pointer to rpi_firmware structure. Is not removing this line going to create a kernel doc warning? With that fixed: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On Mon, Nov 23, 2020 at 7:38 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. > > 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(-) > > diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c > index 30259dc9b805..ed793aef7851 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); One nit from my side: maybe add a comment here saying that you really want to use non-managed kzalloc() because you're going to get people blindly converting it to devm_kzalloc() very soon. Bartosz > + 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; > -- > 2.29.2 >
On Thu, 2020-12-03 at 09:05 +0100, Bartosz Golaszewski wrote: > On Mon, Nov 23, 2020 at 7:38 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. > > > > 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(-) > > > > diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c > > index 30259dc9b805..ed793aef7851 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); > > One nit from my side: maybe add a comment here saying that you really > want to use non-managed kzalloc() because you're going to get people > blindly converting it to devm_kzalloc() very soon. Good point, I'll change it. Regards, Nicolas
diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c index 30259dc9b805..ed793aef7851 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(-)