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

[Eugen Hristev <ehristev@kernel.org>]

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@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 | 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.

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) .
> 
> With or without this small change:
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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,
>> --
>> Eugen Hristev <ehristev@kernel.org>
>>
>