Re: [PATCH v2 03/10] gpio: raspberrypi-exp: Release firmware handle on unbind
On Mon, Oct 26, 2020 at 3:42 PM Nicolas Saenz Julienne
wrote:
>
> On Mon, 2020-10-26 at 15:40 +0100, Bartosz Golaszewski wrote:
> > On Thu, Oct 22, 2020 at 5:59 PM Nicolas Saenz Julienne
> > wrote:
> > > Upon unbinding the device make sure we release RPi's firmware interface.
> > >
> > > Signed-off-by: Nicolas Saenz Julienne
> > > ---
> > > drivers/gpio/gpio-raspberrypi-exp.c | 14 +-
> > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpio-raspberrypi-exp.c
> > > b/drivers/gpio/gpio-raspberrypi-exp.c
> > > index bb100e0124e6..c008336e1131 100644
> > > --- a/drivers/gpio/gpio-raspberrypi-exp.c
> > > +++ b/drivers/gpio/gpio-raspberrypi-exp.c
> > > @@ -231,8 +231,19 @@ static int rpi_exp_gpio_probe(struct platform_device
> > > *pdev)
> > > rpi_gpio->gc.get = rpi_exp_gpio_get;
> > > rpi_gpio->gc.set = rpi_exp_gpio_set;
> > > rpi_gpio->gc.can_sleep = true;
> > > + platform_set_drvdata(pdev, rpi_gpio);
> > >
> > > - return devm_gpiochip_add_data(dev, &rpi_gpio->gc, rpi_gpio);
> > > + return gpiochip_add_data(&rpi_gpio->gc, rpi_gpio);
> > > +}
> > > +
> > > +static int rpi_exp_gpio_remove(struct platform_device *pdev)
> > > +{
> > > + struct rpi_exp_gpio *rpi_gpio = platform_get_drvdata(pdev);
> > > +
> > > + gpiochip_remove(&rpi_gpio->gc);
> > > + rpi_firmware_put(rpi_gpio->fw);
> > > +
> > > + return 0;
> > > }
> > >
> > > static const struct of_device_id rpi_exp_gpio_ids[] = {
> > > @@ -247,6 +258,7 @@ static struct platform_driver rpi_exp_gpio_driver = {
> > > .of_match_table = of_match_ptr(rpi_exp_gpio_ids),
> > > },
> > > .probe = rpi_exp_gpio_probe,
> > > + .remove = rpi_exp_gpio_remove,
> > > };
> > > module_platform_driver(rpi_exp_gpio_driver);
> > >
> > > --
> > > 2.28.0
> > >
> >
> > Why not introduce devm_rpi_firmware_get()? That would allow you to
> > keep the driver elegant without re-adding remove().
>
> I like the idea, I'll look into it.
>
> Thanks,
> Nicolas
>
If you can't do it for some reason, then even using devm_add_action() is fine.
Bartosz
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 03/10] gpio: raspberrypi-exp: Release firmware handle on unbind
On Thu, Oct 22, 2020 at 5:59 PM Nicolas Saenz Julienne
wrote:
>
> Upon unbinding the device make sure we release RPi's firmware interface.
>
> Signed-off-by: Nicolas Saenz Julienne
> ---
> drivers/gpio/gpio-raspberrypi-exp.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-raspberrypi-exp.c
> b/drivers/gpio/gpio-raspberrypi-exp.c
> index bb100e0124e6..c008336e1131 100644
> --- a/drivers/gpio/gpio-raspberrypi-exp.c
> +++ b/drivers/gpio/gpio-raspberrypi-exp.c
> @@ -231,8 +231,19 @@ static int rpi_exp_gpio_probe(struct platform_device
> *pdev)
> rpi_gpio->gc.get = rpi_exp_gpio_get;
> rpi_gpio->gc.set = rpi_exp_gpio_set;
> rpi_gpio->gc.can_sleep = true;
> + platform_set_drvdata(pdev, rpi_gpio);
>
> - return devm_gpiochip_add_data(dev, &rpi_gpio->gc, rpi_gpio);
> + return gpiochip_add_data(&rpi_gpio->gc, rpi_gpio);
> +}
> +
> +static int rpi_exp_gpio_remove(struct platform_device *pdev)
> +{
> + struct rpi_exp_gpio *rpi_gpio = platform_get_drvdata(pdev);
> +
> + gpiochip_remove(&rpi_gpio->gc);
> + rpi_firmware_put(rpi_gpio->fw);
> +
> + return 0;
> }
>
> static const struct of_device_id rpi_exp_gpio_ids[] = {
> @@ -247,6 +258,7 @@ static struct platform_driver rpi_exp_gpio_driver = {
> .of_match_table = of_match_ptr(rpi_exp_gpio_ids),
> },
> .probe = rpi_exp_gpio_probe,
> + .remove = rpi_exp_gpio_remove,
> };
> module_platform_driver(rpi_exp_gpio_driver);
>
> --
> 2.28.0
>
Why not introduce devm_rpi_firmware_get()? That would allow you to
keep the driver elegant without re-adding remove().
Bartosz
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 03/11] gpio: raspberrypi-exp: Release firmware handle on unbind
On Wed, Nov 4, 2020 at 11:39 AM Nicolas Saenz Julienne wrote: > > Use devm_rpi_firmware_get() so as to make sure we release RPi's firmware > interface when unbinding the device. > > Signed-off-by: Nicolas Saenz Julienne > > --- > > Changes since v2: > - Use devm_rpi_firmware_get(), instead of remove function > > drivers/gpio/gpio-raspberrypi-exp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-raspberrypi-exp.c > b/drivers/gpio/gpio-raspberrypi-exp.c > index bb100e0124e6..64a552ecc2ad 100644 > --- a/drivers/gpio/gpio-raspberrypi-exp.c > +++ b/drivers/gpio/gpio-raspberrypi-exp.c > @@ -208,7 +208,7 @@ static int rpi_exp_gpio_probe(struct platform_device > *pdev) > return -ENOENT; > } > > - fw = rpi_firmware_get(fw_node); > + fw = devm_rpi_firmware_get(&pdev->dev, fw_node); > of_node_put(fw_node); > if (!fw) > return -EPROBE_DEFER; > -- > 2.29.1 > Acked-by: Bartosz Golaszewski ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 01/11] firmware: raspberrypi: Introduce devm_rpi_firmware_get()
On Wed, Nov 4, 2020 at 11:39 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
> devm_rpi_firmware_get() which will automatically decrease the reference
> count upon unbinding consumer drivers.
>
> Suggested-by: Uwe Kleine-König
> Signed-off-by: Nicolas Saenz Julienne
>
> ---
>
> Changes since v2:
> - Create devm_rpi_firmware_get()
>
> drivers/firmware/raspberrypi.c | 46 ++
> include/soc/bcm2835/raspberrypi-firmware.h | 8
> 2 files changed, 54 insertions(+)
>
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index 2371d08bdd17..74bdb3bde9dc 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
> @@ -11,7 +11,9 @@
> #include
> #include
> #include
> +#include
> #include
> +#include
> #include
>
> #define MBOX_MSG(chan, data28) (((data28) & ~0xf) | ((chan) & 0xf))
> @@ -27,6 +29,9 @@ struct rpi_firmware {
> struct mbox_chan *chan; /* The property channel. */
> struct completion c;
> u32 enabled;
> +
> + refcount_t consumers;
> + wait_queue_head_t wait;
> };
>
> static DEFINE_MUTEX(transaction_lock);
> @@ -247,6 +252,8 @@ static int rpi_firmware_probe(struct platform_device
> *pdev)
> }
>
> init_completion(&fw->c);
> + refcount_set(&fw->consumers, 1);
> + init_waitqueue_head(&fw->wait);
>
> platform_set_drvdata(pdev, fw);
>
> @@ -275,11 +282,21 @@ static int rpi_firmware_remove(struct platform_device
> *pdev)
> rpi_hwmon = NULL;
> platform_device_unregister(rpi_clk);
> rpi_clk = NULL;
> +
> + wait_event(fw->wait, refcount_dec_if_one(&fw->consumers));
> mbox_free_channel(fw->chan);
>
> return 0;
> }
>
> +static void rpi_firmware_put(void *data)
> +{
> + struct rpi_firmware *fw = data;
> +
> + refcount_dec(&fw->consumers);
> + wake_up(&fw->wait);
> +}
> +
> /**
> * rpi_firmware_get - Get pointer to rpi_firmware structure.
> * @firmware_node:Pointer to the firmware Device Tree node.
> @@ -297,6 +314,35 @@ struct rpi_firmware *rpi_firmware_get(struct device_node
> *firmware_node)
> }
> EXPORT_SYMBOL_GPL(rpi_firmware_get);
>
> +/**
> + * devm_rpi_firmware_get - Get pointer to rpi_firmware structure.
> + * @firmware_node:Pointer to the firmware Device Tree node.
> + *
> + * Returns NULL is the firmware device is not ready.
> + */
> +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> + struct device_node *firmware_node)
> +{
> + struct platform_device *pdev = of_find_device_by_node(firmware_node);
> + struct rpi_firmware *fw;
> +
> + if (!pdev)
> + return NULL;
> +
> + fw = platform_get_drvdata(pdev);
> + if (!fw)
> + return NULL;
> +
> + if (!refcount_inc_not_zero(&fw->consumers))
> + return NULL;
> +
> + if (devm_add_action_or_reset(dev, rpi_firmware_put, fw))
> + return NULL;
> +
> + return fw;
> +}
> +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get);
Usually I'd expect the devres variant to simply call
rpi_firmware_get() and then schedule a release callback which would
call whatever function is the release counterpart for it currently.
Devres actions are for drivers which want to schedule some more
unusual tasks at driver detach. Any reason for designing it this way?
Bartosz
> +
> static const struct of_device_id rpi_firmware_of_match[] = {
> { .compatible = "raspberrypi,bcm2835-firmware", },
> {},
> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h
> b/include/soc/bcm2835/raspberrypi-firmware.h
> index cc9cdbc66403..8fe64f53a394 100644
> --- a/include/soc/bcm2835/raspberrypi-firmware.h
> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
> @@ -141,6 +141,8 @@ int rpi_firmware_property(struct rpi_firmware *fw,
> int rpi_firmware_property_list(struct rpi_firmware *fw,
>void *data, size_t tag_size);
> struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node);
> +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> + struct device_node *firmware_node);
> #else
> static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag,
> void *data, size_t len)
> @@ -158,6 +160,12 @@ static inline struct rpi_firmware
> *rpi_firmware_get(struct device_node *firmware
> {
> return NULL;
> }
> +
> +static inline struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> + struct device_node *firmware_node)
> +{
> + return NULL;
> +}
> #endif
>
> #endif /* __SOC_RA
Re: [PATCH v3 01/11] firmware: raspberrypi: Introduce devm_rpi_firmware_get()
On Thu, Nov 5, 2020 at 10:28 AM Nicolas Saenz Julienne
wrote:
>
> Hi Bartosz, thanks for the review.
>
> On Thu, 2020-11-05 at 10:13 +0100, Bartosz Golaszewski wrote:
> > > +/**
> > > + * devm_rpi_firmware_get - Get pointer to rpi_firmware structure.
> > > + * @firmware_node:Pointer to the firmware Device Tree node.
> > > + *
> > > + * Returns NULL is the firmware device is not ready.
> > > + */
> > > +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> > > + struct device_node
> > > *firmware_node)
> > > +{
> > > + struct platform_device *pdev =
> > > of_find_device_by_node(firmware_node);
> > > + struct rpi_firmware *fw;
> > > +
> > > + if (!pdev)
> > > + return NULL;
> > > +
> > > + fw = platform_get_drvdata(pdev);
> > > + if (!fw)
> > > + return NULL;
> > > +
> > > + if (!refcount_inc_not_zero(&fw->consumers))
> > > + return NULL;
> > > +
> > > + if (devm_add_action_or_reset(dev, rpi_firmware_put, fw))
> > > + return NULL;
> > > +
> > > + return fw;
> > > +}
> > > +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get);
> >
> > Usually I'd expect the devres variant to simply call
> > rpi_firmware_get() and then schedule a release callback which would
> > call whatever function is the release counterpart for it currently.
> > Devres actions are for drivers which want to schedule some more
> > unusual tasks at driver detach. Any reason for designing it this way?
>
> Yes, see patch #8 where I get rid of rpi_firmware_get() altogether after
> converting all users to devres. Since there is no use for the vanilla version
> of the function anymore, I figured it'd be better to merge everything into
> devm_rpi_firmware_get(). That said it's not something I have strong feelings
> about.
>
I see. So the previous version didn't really have any reference
counting and it leaked the reference returned by
of_find_device_by_node(), got it. Could you just clarify for me the
logic behind the wait_queue in rpi_firmware_remove()? If the firmware
driver gets detached and remove() stops on the wait_queue - it will be
stuck until the last user releases the firmware. I'm not sure this is
correct. I'd prefer to see a kref with a release callback and remove
would simply decrease the kref and return. Each user would do the same
and then after the last user is detached the firmware would be
destroyed.
Don't we really have some centralized firmware subsystem that would handle this?
Bartosz
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 02/11] firmware: raspberrypi: Introduce devm_rpi_firmware_get()
On Thu, Nov 12, 2020 at 5:44 PM Nicolas Saenz Julienne
wrote:
>
> Itroduce devm_rpi_firmware_get(), it'll simplify the firmware handling
> for most consumers.
>
> Suggested-by: Bartosz Golaszewski
> Signed-off-by: Nicolas Saenz Julienne
> ---
>
> Changes since v2:
> - Introduce devm_rpi_firmware_get()
>
> drivers/firmware/raspberrypi.c | 31 +-
> include/soc/bcm2835/raspberrypi-firmware.h | 8 ++
> 2 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index 438e17074a97..4ab2dfdc82ad 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
> @@ -237,10 +237,17 @@ static void rpi_firmware_delete(struct kref *kref)
> kfree(fw);
> }
>
> -void rpi_firmware_put(struct rpi_firmware *fw)
> +static void __rpi_firmware_put(void *data)
> {
The '__' prefix is very vague and usually used for unlocked variants
of functions. The casting to void * in rpi_firmware_put() is also
unneeded. I would much prefer that the devres release callback be
called devm_rpi_firmware_put() and that it call rpi_firmware_put()
which would then call kref_put().
Bartosz
> + struct rpi_firmware *fw = data;
> +
> kref_put(&fw->consumers, rpi_firmware_delete);
> }
> +
> +void rpi_firmware_put(struct rpi_firmware *fw)
> +{
> + __rpi_firmware_put(fw);
> +}
> EXPORT_SYMBOL_GPL(rpi_firmware_put);
>
> static int rpi_firmware_probe(struct platform_device *pdev)
> @@ -326,6 +333,28 @@ struct rpi_firmware *rpi_firmware_get(struct device_node
> *firmware_node)
> }
> EXPORT_SYMBOL_GPL(rpi_firmware_get);
>
> +/**
> + * devm_rpi_firmware_get - Get pointer to rpi_firmware structure.
> + * @firmware_node:Pointer to the firmware Device Tree node.
> + *
> + * Returns NULL is the firmware device is not ready.
> + */
> +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> + struct device_node *firmware_node)
> +{
> + struct rpi_firmware *fw;
> +
> + fw = rpi_firmware_get(firmware_node);
> + if (!fw)
> + return NULL;
> +
> + if (devm_add_action_or_reset(dev, __rpi_firmware_put, fw))
> + return NULL;
> +
> + return fw;
> +}
> +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get);
> +
> static const struct of_device_id rpi_firmware_of_match[] = {
> { .compatible = "raspberrypi,bcm2835-firmware", },
> {},
> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h
> b/include/soc/bcm2835/raspberrypi-firmware.h
> index fdfef7fe40df..73ad784fca96 100644
> --- a/include/soc/bcm2835/raspberrypi-firmware.h
> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
> @@ -142,6 +142,8 @@ 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);
> +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> + struct device_node *firmware_node);
> #else
> static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag,
> void *data, size_t len)
> @@ -160,6 +162,12 @@ static inline struct rpi_firmware
> *rpi_firmware_get(struct device_node *firmware
> {
> return NULL;
> }
> +
> +static inline struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> + struct device_node *firmware_node)
> +{
> + return NULL;
> +}
> #endif
>
> #endif /* __SOC_RASPBERRY_FIRMWARE_H__ */
> --
> 2.29.2
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 01/11] firmware: raspberrypi: Keep count of all consumers
On Mon, Nov 23, 2020 at 7:38 PM 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
> Signed-off-by: Nicolas Saenz Julienne
> ---
>
> 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
> +#include
> #include
> #include
> #include
> @@ -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
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 02/11] firmware: raspberrypi: Introduce devm_rpi_firmware_get()
On Fri, Dec 11, 2020 at 5:48 PM Nicolas Saenz Julienne
wrote:
>
> It'll simplify the firmware handling for most consumers.
>
> Suggested-by: Bartosz Golaszewski
> Signed-off-by: Nicolas Saenz Julienne
> Reviewed-by: Florian Fainelli
> ---
>
> Changes since v4:
> - Rearrange function calls for clarity, same functionality
>
> Changes since v2:
> - Create devm_rpi_firmware_get()
>
> drivers/firmware/raspberrypi.c | 29 ++
> include/soc/bcm2835/raspberrypi-firmware.h | 8 ++
> 2 files changed, 37 insertions(+)
>
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index b65e4c495772..250e01680742 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
> @@ -243,6 +243,13 @@ void rpi_firmware_put(struct rpi_firmware *fw)
> }
> EXPORT_SYMBOL_GPL(rpi_firmware_put);
>
> +static void devm_rpi_firmware_put(void *data)
> +{
> + struct rpi_firmware *fw = data;
> +
> + rpi_firmware_put(fw);
> +}
> +
> static int rpi_firmware_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -331,6 +338,28 @@ struct rpi_firmware *rpi_firmware_get(struct device_node
> *firmware_node)
> }
> EXPORT_SYMBOL_GPL(rpi_firmware_get);
>
> +/**
> + * devm_rpi_firmware_get - Get pointer to rpi_firmware structure.
> + * @firmware_node:Pointer to the firmware Device Tree node.
> + *
> + * Returns NULL is the firmware device is not ready.
> + */
> +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> + struct device_node *firmware_node)
> +{
> + struct rpi_firmware *fw;
> +
> + fw = rpi_firmware_get(firmware_node);
> + if (!fw)
> + return NULL;
> +
> + if (devm_add_action_or_reset(dev, devm_rpi_firmware_put, fw))
> + return NULL;
> +
> + return fw;
> +}
> +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get);
> +
> static const struct of_device_id rpi_firmware_of_match[] = {
> { .compatible = "raspberrypi,bcm2835-firmware", },
> {},
> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h
> b/include/soc/bcm2835/raspberrypi-firmware.h
> index fdfef7fe40df..73ad784fca96 100644
> --- a/include/soc/bcm2835/raspberrypi-firmware.h
> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
> @@ -142,6 +142,8 @@ 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);
> +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> + struct device_node *firmware_node);
> #else
> static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag,
> void *data, size_t len)
> @@ -160,6 +162,12 @@ static inline struct rpi_firmware
> *rpi_firmware_get(struct device_node *firmware
> {
> return NULL;
> }
> +
> +static inline struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> + struct device_node *firmware_node)
> +{
> + return NULL;
> +}
> #endif
>
> #endif /* __SOC_RASPBERRY_FIRMWARE_H__ */
> --
> 2.29.2
>
Reviewed-by: Bartosz Golaszewski
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 01/11] firmware: raspberrypi: Keep count of all consumers
On Fri, Dec 11, 2020 at 5:48 PM 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 > Signed-off-by: Nicolas Saenz Julienne > Reviewed-by: Florian Fainelli > > --- Reviewed-by: Bartosz Golaszewski ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 1/3] gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
niedz., 15 gru 2019 o 19:31 Dmitry Osipenko napisał(a):
>
> There is no point in using old-style raw accessors, the generic accessors
> do the same thing and also take into account CPU endianness. Tegra SoCs do
> not support big-endian mode in the upstream kernel, but let's switch away
> from the outdated things anyway, just to keep code up-to-date.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpio/gpio-tegra.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> index 6fdfe4c5303e..f6a382fbd12d 100644
> --- a/drivers/gpio/gpio-tegra.c
> +++ b/drivers/gpio/gpio-tegra.c
> @@ -96,12 +96,12 @@ struct tegra_gpio_info {
> static inline void tegra_gpio_writel(struct tegra_gpio_info *tgi,
> u32 val, u32 reg)
> {
> - __raw_writel(val, tgi->regs + reg);
> + writel_relaxed(val, tgi->regs + reg);
> }
>
> static inline u32 tegra_gpio_readl(struct tegra_gpio_info *tgi, u32 reg)
> {
> - return __raw_readl(tgi->regs + reg);
> + return readl_relaxed(tgi->regs + reg);
> }
>
> static unsigned int tegra_gpio_compose(unsigned int bank, unsigned int port,
> --
> 2.24.0
>
The entire series looks good to me, but I'll wait for Thierry's acks
just in case.
Bart
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 0/3] Tegra GPIO: Minor code clean up
pon., 6 sty 2020 o 23:59 Linus Walleij napisał(a): > > On Sun, Dec 15, 2019 at 7:31 PM Dmitry Osipenko wrote: > > > I was investigating why CPU hangs during of GPIO driver suspend and in > > the end it turned out that it is a Broadcom WiFi driver problem because > > it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and this > > may cause a bit weird CPU hang on writing to INT_ENB register during of > > GPIO driver suspend. Meanwhile I also noticed that a few things could be > > improved in the driver, that's what this small series addresses. > > > > Dmitry Osipenko (3): > > gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors > > gpio: tegra: Properly handle irq_set_irq_wake() error > > gpio: tegra: Use NOIRQ phase for suspend/resume > > All three patches applied with Thierry's review/test tag. > > Yours, > Linus Walleij Ugh, I now noticed I responded to Thierry only after applying this to my tree. Anyway, it shouldn't be a problem. I'll take more care next time. Bart ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 0/3] Tegra GPIO: Minor code clean up
wt., 7 sty 2020 o 10:29 Linus Walleij napisał(a): > > On Tue, Jan 7, 2020 at 9:25 AM Bartosz Golaszewski > wrote: > > > pon., 6 sty 2020 o 23:59 Linus Walleij > > napisał(a): > > > On Sun, Dec 15, 2019 at 7:31 PM Dmitry Osipenko wrote: > > > > > > > I was investigating why CPU hangs during of GPIO driver suspend and in > > > > the end it turned out that it is a Broadcom WiFi driver problem because > > > > it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and > > > > this > > > > may cause a bit weird CPU hang on writing to INT_ENB register during of > > > > GPIO driver suspend. Meanwhile I also noticed that a few things could be > > > > improved in the driver, that's what this small series addresses. > > > > > > > > Dmitry Osipenko (3): > > > > gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors > > > > gpio: tegra: Properly handle irq_set_irq_wake() error > > > > gpio: tegra: Use NOIRQ phase for suspend/resume > > > > > > All three patches applied with Thierry's review/test tag. > > > > > > Yours, > > > Linus Walleij > > > > Ugh, I now noticed I responded to Thierry only after applying this to my > > tree. > > > > Anyway, it shouldn't be a problem. I'll take more care next time. > > OK shall I drop the patches from my tree then? No big deal. > If you're fine with this, sure! Thanks a lot, Bart ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
