[PATCH v2] media: bcm2835-unicam: Fix log status runtime access

[Jai Luthra]

Hi Sakari, Eugen,

Quoting Sakari Ailus (2026-06-01 02:32:09)
> Hi Eugen, others,
> 
> On Fri, May 29, 2026 at 06:06:42PM +0300, Eugen Hristev wrote:
> > On 5/29/26 08:12, Jean-Michel Hautbois wrote:
> > > Hi Eugen,
> > > 
> > > Le 22/05/2026 à 17:28, Eugen Hristev a écrit :
> > >> When requesting log status, the block might be powered off, but registers
> > >> are being read.
> > >> Avoid reading the registers if the device is not resumed, thus also avoid
> > >> powering up the device just for log status.
> > >>
> > >> Fixes: 392cd78d495f ("media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface")
> > >> Signed-off-by: Eugen Hristev <ehristev at kernel.org>
> > >> ---
> > >> Changes in v2:
> > >> - changed to use pm_runtime_get_if_active()
> > >> - add corresponding put()
> > >> - Link to v1: https://patch.msgid.link/20260521-bcmpipm-v1-1-3eba88d88045@kernel.org
> > >>
> > >> To: Raspberry Pi Kernel Maintenance <kernel-list at raspberrypi.com>
> > >> To: Mauro Carvalho Chehab <mchehab at kernel.org>
> > >> To: Florian Fainelli <florian.fainelli at broadcom.com>
> > >> To: Ray Jui <rjui at broadcom.com>
> > >> To: Scott Branden <sbranden at broadcom.com>
> > >> To: Broadcom internal kernel review list <bcm-kernel-feedback-list at broadcom.com>
> > >> To: Sakari Ailus <sakari.ailus at linux.intel.com>
> > >> To: Jean-Michel Hautbois <jeanmichel.hautbois at yoseli.org>
> > >> To: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >> To: Hans Verkuil <hverkuil at kernel.org>
> > >> To: Naushir Patuck <naush at raspberrypi.com>
> > >> Cc: Dave Stevenson <dave.stevenson at raspberrypi.com>
> > >> Cc: linux-media at vger.kernel.org
> > >> Cc: linux-rpi-kernel at lists.infradead.org
> > >> Cc: linux-arm-kernel at lists.infradead.org
> > >> Cc: linux-kernel at vger.kernel.org
> > >> ---
> > >>   drivers/media/platform/broadcom/bcm2835-unicam.c | 9 +++++++++
> > >>   1 file changed, 9 insertions(+)
> > >>
> > >> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > >> index 8d28ba0b59a3..93815b8ab930 100644
> > >> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > >> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > >> @@ -2052,6 +2052,13 @@ static int unicam_log_status(struct file *file, void *fh)
> > >>             node->fmt.fmt.pix.width, node->fmt.fmt.pix.height);
> > >>    dev_info(unicam->dev, "V4L2 format:         %08x\n",
> > >>             node->fmt.fmt.pix.pixelformat);
> > >> +
> > >> +  if (!pm_runtime_get_if_active(unicam->dev)) {
> > > 
> > > Well, if I am picky I would say that pm_runtime_get_if_active() can 
> > > return -EINVAL if runtime PM is disabled for the device. It should then 
> > > be tested against '<= 0' ?
> > > 
> > > I suppose this should not happen really often, as very few drivers 
> > > actually test this case...
> > 
> > I saw that. Some do. This driver enables runtime pm in probe though. So
> > I guess it cannot happen, unless runtime pm would not selected in kernel
> > config, but the driver depends on PM.
> 
> Runtime PM can be disabled for a device via sysfs.
> 

I tried this on my board, and you can only do:

echo on > /sys/class/<...>/control

Which leads to RPM count to always stay 1 regardless of get/put.

So if the driver enables RPM and never disables it unless it's removed,
there's no way from userspace to make this function return -EINVAL.

> > 
> > Ultimately I guess it's up to Sakari or Hans to decide whether it's
> > worth checking for error code, but I picked the simpler path (and
> > considering <depends on PM> in Kconfig) .
> 
> pm_runtime_put() musn't be called if there was an error as it decrements
> usage_count unconditionally.
> 
> The vast majority of sensor drivers test for non-zero only and no-one has
> complained. They should be fixed though...
> 

This driver has an explicit "depends on PM", but even for other sensor
drivers, the only case pm_runtime_get_if_active can return -EINVAL is when
CONFIG_PM=n, where pm_runtime_put is also stubbed out to return -ENOSYS..

So there is nothing to fix in sesnor drivers which do:

    if (!pm_runtime_get_if_active(&client->dev))
            return 0;

    /* write registers */

    /* unconditional put */
    pm_runtime_put(&client->dev)

Thanks,
    Jai

> > > 
> > > With or without this small change:
> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at yoseli.org>
> > > 
> > > Thanks,
> > > JM
> > > 
> > >> +          dev_info(unicam->dev,
> > >> +                   "Live data N/A due to device inactive\n");
> > >> +          return 0;
> > >> +  }
> > >> +
> > >>    reg = unicam_reg_read(unicam, UNICAM_IPIPE);
> > >>    dev_info(unicam->dev, "Unpacking/packing:   %u / %u\n",
> > >>             unicam_get_field(reg, UNICAM_PUM_MASK),
> > >> @@ -2065,6 +2072,8 @@ static int unicam_log_status(struct file *file, void *fh)
> > >>    dev_info(unicam->dev, "Write pointer:       %08x\n",
> > >>             unicam_reg_read(unicam, UNICAM_IBWP));
> > >>   
> > >> +  pm_runtime_put(unicam->dev);
> > >> +
> > >>    return 0;
> > >>   }
> > >>   
> > >>
> > >> ---
> > >> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
> > >> change-id: 20260521-bcmpipm-6c578e73239c
> > >>
> > >> Best regards,
> 
> -- 
> Regards,
> 
> Sakari Ailus
>