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

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

[Eugen Hristev]

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@kernel.org>
---
Changes in v3:
- Changed to check return value of pm_runtime_get_if_active() and only call
  pm_runtime_put() if the device is active.
- Link to v2: https://patch.msgid.link/20260522-bcmpipm-v2-1-a3da66cbc9f0@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@raspberrypi.com>
To: Mauro Carvalho Chehab <mchehab@kernel.org>
To: Florian Fainelli <florian.fainelli@broadcom.com>
To: Ray Jui <rjui@broadcom.com>
To: Scott Branden <sbranden@broadcom.com>
To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Hans Verkuil <hverkuil@kernel.org>
To: Naushir Patuck <naush@raspberrypi.com>
Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: linux-media@vger.kernel.org
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/media/platform/broadcom/bcm2835-unicam.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
index 8d28ba0b59a3..96b51e29bba4 100644
--- a/drivers/media/platform/broadcom/bcm2835-unicam.c
+++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
@@ -2043,6 +2043,7 @@ static int unicam_log_status(struct file *file, void *fh)
 	struct unicam_node *node = video_drvdata(file);
 	struct unicam_device *unicam = node->dev;
 	u32 reg;
+	int pm_active;
 
 	/* status for sub devices */
 	v4l2_device_call_all(&unicam->v4l2_dev, 0, core, log_status);
@@ -2052,6 +2053,14 @@ 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);
+
+	pm_active = pm_runtime_get_if_active(unicam->dev);
+	if (!pm_active) {
+		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 +2074,9 @@ static int unicam_log_status(struct file *file, void *fh)
 	dev_info(unicam->dev, "Write pointer:       %08x\n",
 		 unicam_reg_read(unicam, UNICAM_IBWP));
 
+	if (pm_active == 1)
+		pm_runtime_put(unicam->dev);
+
 	return 0;
 }
 

---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260521-bcmpipm-6c578e73239c

Best regards,
--  
Eugen Hristev <ehristev@kernel.org>

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

[Laurent Pinchart]

On Thu, Jun 11, 2026 at 08:29:55AM +0300, Eugen Hristev wrote:
> 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@kernel.org>
> ---
> Changes in v3:
> - Changed to check return value of pm_runtime_get_if_active() and only call
>   pm_runtime_put() if the device is active.
> - Link to v2: https://patch.msgid.link/20260522-bcmpipm-v2-1-a3da66cbc9f0@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@raspberrypi.com>
> To: Mauro Carvalho Chehab <mchehab@kernel.org>
> To: Florian Fainelli <florian.fainelli@broadcom.com>
> To: Ray Jui <rjui@broadcom.com>
> To: Scott Branden <sbranden@broadcom.com>
> To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
> To: Sakari Ailus <sakari.ailus@linux.intel.com>
> To: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> To: Hans Verkuil <hverkuil@kernel.org>
> To: Naushir Patuck <naush@raspberrypi.com>
> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: linux-media@vger.kernel.org
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/media/platform/broadcom/bcm2835-unicam.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> index 8d28ba0b59a3..96b51e29bba4 100644
> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -2043,6 +2043,7 @@ static int unicam_log_status(struct file *file, void *fh)
>  	struct unicam_node *node = video_drvdata(file);
>  	struct unicam_device *unicam = node->dev;
>  	u32 reg;
> +	int pm_active;
>  
>  	/* status for sub devices */
>  	v4l2_device_call_all(&unicam->v4l2_dev, 0, core, log_status);
> @@ -2052,6 +2053,14 @@ 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);
> +
> +	pm_active = pm_runtime_get_if_active(unicam->dev);
> +	if (!pm_active) {
> +		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 +2074,9 @@ static int unicam_log_status(struct file *file, void *fh)
>  	dev_info(unicam->dev, "Write pointer:       %08x\n",
>  		 unicam_reg_read(unicam, UNICAM_IBWP));
>  
> +	if (pm_active == 1)
> +		pm_runtime_put(unicam->dev);

As far as I understand, the discussion on v2 concluded there was no need
to test pm_active here. Did I miss anything ?

> +
>  	return 0;
>  }
>  
> 
> ---
> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
> change-id: 20260521-bcmpipm-6c578e73239c

-- 
Regards,

Laurent Pinchart

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

[Eugen Hristev]

On 6/11/26 11:03, Laurent Pinchart wrote:
> On Thu, Jun 11, 2026 at 08:29:55AM +0300, Eugen Hristev wrote:
>> 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@kernel.org>
>> ---
>> Changes in v3:
>> - Changed to check return value of pm_runtime_get_if_active() and only call
>>   pm_runtime_put() if the device is active.
>> - Link to v2: https://patch.msgid.link/20260522-bcmpipm-v2-1-a3da66cbc9f0@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@raspberrypi.com>
>> To: Mauro Carvalho Chehab <mchehab@kernel.org>
>> To: Florian Fainelli <florian.fainelli@broadcom.com>
>> To: Ray Jui <rjui@broadcom.com>
>> To: Scott Branden <sbranden@broadcom.com>
>> To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
>> To: Sakari Ailus <sakari.ailus@linux.intel.com>
>> To: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
>> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> To: Hans Verkuil <hverkuil@kernel.org>
>> To: Naushir Patuck <naush@raspberrypi.com>
>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> Cc: linux-media@vger.kernel.org
>> Cc: linux-rpi-kernel@lists.infradead.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  drivers/media/platform/broadcom/bcm2835-unicam.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
>> index 8d28ba0b59a3..96b51e29bba4 100644
>> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
>> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
>> @@ -2043,6 +2043,7 @@ static int unicam_log_status(struct file *file, void *fh)
>>  	struct unicam_node *node = video_drvdata(file);
>>  	struct unicam_device *unicam = node->dev;
>>  	u32 reg;
>> +	int pm_active;
>>  
>>  	/* status for sub devices */
>>  	v4l2_device_call_all(&unicam->v4l2_dev, 0, core, log_status);
>> @@ -2052,6 +2053,14 @@ 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);
>> +
>> +	pm_active = pm_runtime_get_if_active(unicam->dev);
>> +	if (!pm_active) {
>> +		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 +2074,9 @@ static int unicam_log_status(struct file *file, void *fh)
>>  	dev_info(unicam->dev, "Write pointer:       %08x\n",
>>  		 unicam_reg_read(unicam, UNICAM_IBWP));
>>  
>> +	if (pm_active == 1)
>> +		pm_runtime_put(unicam->dev);
> 
> As far as I understand, the discussion on v2 concluded there was no need
> to test pm_active here. Did I miss anything ?

Sorry, I saw the message from Sakari and he was pretty confident on the
right way, he even mentioned that all sensors should be fixed, and has
not come up with a follow up since.
If v2 is the right way, please disregard this v3

Eugen

> 
>> +
>>  	return 0;
>>  }
>>  
>>
>> ---
>> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
>> change-id: 20260521-bcmpipm-6c578e73239c
> 

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

[Laurent Pinchart]

On Thu, Jun 11, 2026 at 08:18:10PM +0300, Eugen Hristev wrote:
> On 6/11/26 11:03, Laurent Pinchart wrote:
> > On Thu, Jun 11, 2026 at 08:29:55AM +0300, Eugen Hristev wrote:
> >> 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@kernel.org>
> >> ---
> >> Changes in v3:
> >> - Changed to check return value of pm_runtime_get_if_active() and only call
> >>   pm_runtime_put() if the device is active.
> >> - Link to v2: https://patch.msgid.link/20260522-bcmpipm-v2-1-a3da66cbc9f0@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@raspberrypi.com>
> >> To: Mauro Carvalho Chehab <mchehab@kernel.org>
> >> To: Florian Fainelli <florian.fainelli@broadcom.com>
> >> To: Ray Jui <rjui@broadcom.com>
> >> To: Scott Branden <sbranden@broadcom.com>
> >> To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
> >> To: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> To: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
> >> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> To: Hans Verkuil <hverkuil@kernel.org>
> >> To: Naushir Patuck <naush@raspberrypi.com>
> >> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >> Cc: linux-media@vger.kernel.org
> >> Cc: linux-rpi-kernel@lists.infradead.org
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Cc: linux-kernel@vger.kernel.org
> >> ---
> >>  drivers/media/platform/broadcom/bcm2835-unicam.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> >> index 8d28ba0b59a3..96b51e29bba4 100644
> >> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> >> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> >> @@ -2043,6 +2043,7 @@ static int unicam_log_status(struct file *file, void *fh)
> >>  	struct unicam_node *node = video_drvdata(file);
> >>  	struct unicam_device *unicam = node->dev;
> >>  	u32 reg;
> >> +	int pm_active;
> >>  
> >>  	/* status for sub devices */
> >>  	v4l2_device_call_all(&unicam->v4l2_dev, 0, core, log_status);
> >> @@ -2052,6 +2053,14 @@ 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);
> >> +
> >> +	pm_active = pm_runtime_get_if_active(unicam->dev);
> >> +	if (!pm_active) {
> >> +		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 +2074,9 @@ static int unicam_log_status(struct file *file, void *fh)
> >>  	dev_info(unicam->dev, "Write pointer:       %08x\n",
> >>  		 unicam_reg_read(unicam, UNICAM_IBWP));
> >>  
> >> +	if (pm_active == 1)
> >> +		pm_runtime_put(unicam->dev);
> > 
> > As far as I understand, the discussion on v2 concluded there was no need
> > to test pm_active here. Did I miss anything ?
> 
> Sorry, I saw the message from Sakari and he was pretty confident on the
> right way, he even mentioned that all sensors should be fixed, and has
> not come up with a follow up since.
> If v2 is the right way, please disregard this v3

Let's see what Sakari has to say :-)

> >> +
> >>  	return 0;
> >>  }
> >>  
> >>
> >> ---
> >> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
> >> change-id: 20260521-bcmpipm-6c578e73239c

-- 
Regards,

Laurent Pinchart