Hi Bartosz, thanks for the feedback.

On Thu, 2020-11-05 at 10:42 +0100, Bartosz Golaszewski wrote:

On Thu, Nov 5, 2020 at 10:28 AM Nicolas Saenz Julienne
[off-list ref] wrote:

quoted

Hi Bartosz, thanks for the review.

On Thu, 2020-11-05 at 10:13 +0100, Bartosz Golaszewski wrote:

quoted

quoted

+/**
+ * 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.

Yes, that's what I meant to implement.

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.

Sounds good to me. I'll update it.

Don't we really have some centralized firmware subsystem that would handle this?

Sadly no, this is an RPi specific thing, it doesn't live behind a standard like
other firmware based protocols (for ex. SCMI), and evolves as the needs arise.

Regards,
Nicolas