linux-kernel - Re: [PATCH 12/19] media: i2c: ds90ub960: Add RX port iteration support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <92c7ee5b-3495-4398-99dd-881c704c64c1@ideasonboard.com>
Date: Fri, 24 Jan 2025 15:34:44 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org, Devarsh Thakkar <devarsht@...com>,
 Jai Luthra <jai.luthra@...asonboard.com>
Subject: Re: [PATCH 12/19] media: i2c: ds90ub960: Add RX port iteration
 support

Hi,

On 15/01/2025 16:23, Sakari Ailus wrote:
> Moi,
> 
> On Fri, Jan 10, 2025 at 11:14:12AM +0200, Tomi Valkeinen wrote:
>> The driver does a lot of iteration over the RX ports with for loops. In
>> most cases the driver will skip unused RX ports. Also, in the future
>> patches the FPD-Link IV support will be refreshed with TI's latest init
>> sequences which involves a lot of additional iterations over the RX
>> ports, often only for FPD-Link IV ports.
>>
>> To make the iteration simpler and to make it clearer what we're
>> iterating over (all or only-active, all or only-fpd4), add macros and
>> support functions for iterating the RX ports. Use the macros in the
>> driver, replacing the for loops.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>> ---
>>   drivers/media/i2c/ds90ub960.c | 260 ++++++++++++++++++++++--------------------
>>   1 file changed, 135 insertions(+), 125 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
>> index bca858172942..02e22ae813fa 100644
>> --- a/drivers/media/i2c/ds90ub960.c
>> +++ b/drivers/media/i2c/ds90ub960.c
>> @@ -649,6 +649,63 @@ static const struct ub960_format_info *ub960_find_format(u32 code)
>>   	return NULL;
>>   }
>>   
>> +struct ub960_rxport_iter {
>> +	unsigned int nport;
>> +	struct ub960_rxport *rxport;
>> +};
>> +
>> +enum ub960_iter_flags {
>> +	UB960_ITER_ACTIVE_ONLY = BIT(0),
>> +	UB960_ITER_FPD4_ONLY = BIT(1),
>> +};
>> +
>> +static struct ub960_rxport_iter ub960_iter_rxport(struct ub960_data *priv,
>> +						  struct ub960_rxport_iter it,
>> +						  enum ub960_iter_flags flags)
>> +{
>> +	for (; it.nport < priv->hw_data->num_rxports; it.nport++) {
>> +		it.rxport = priv->rxports[it.nport];
>> +
>> +		if ((flags & UB960_ITER_ACTIVE_ONLY) && !it.rxport)
>> +			continue;
>> +
>> +		if ((flags & UB960_ITER_FPD4_ONLY) &&
>> +		    it.rxport->cdr_mode != RXPORT_CDR_FPD4)
>> +			continue;
>> +
>> +		return it;
>> +	}
>> +
>> +	it.rxport = NULL;
>> +
>> +	return it;
>> +}
>> +
>> +#define for_each_rxport(priv)                                                 \
> 
> it should be also an argument to the macro as it's visible outside it.
> 
> And wouldn't it be reasonable to use a pointer instead for the purpsoe?

You mean something like:

   struct ub960_rxport_iter it = { 0 };

   for_each_rxport(priv, &it) { }

Then we leak the iterator, and I really hate it. I've fixed numerous 
bugs caused by such cases.

  Tomi

> 
>> +	for (struct ub960_rxport_iter it =                                    \
>> +		     ub960_iter_rxport(priv, (struct ub960_rxport_iter){ 0 }, \
>> +				       0);                                    \
>> +	     it.nport < (priv)->hw_data->num_rxports;                         \
>> +	     it.nport++, it = ub960_iter_rxport(priv, it, 0))
>> +
>> +#define for_each_active_rxport(priv)                                          \
>> +	for (struct ub960_rxport_iter it =                                    \
>> +		     ub960_iter_rxport(priv, (struct ub960_rxport_iter){ 0 }, \
>> +				       UB960_ITER_ACTIVE_ONLY);               \
>> +	     it.nport < (priv)->hw_data->num_rxports;                         \
>> +	     it.nport++, it = ub960_iter_rxport(priv, it,                     \
>> +						UB960_ITER_ACTIVE_ONLY))
>> +
>> +#define for_each_active_rxport_fpd4(priv)                                     \
>> +	for (struct ub960_rxport_iter it =                                    \
>> +		     ub960_iter_rxport(priv, (struct ub960_rxport_iter){ 0 }, \
>> +				       UB960_ITER_ACTIVE_ONLY |               \
>> +					       UB960_ITER_FPD4_ONLY);         \
>> +	     it.nport < (priv)->hw_data->num_rxports;                         \
>> +	     it.nport++, it = ub960_iter_rxport(priv, it,                     \
>> +						UB960_ITER_ACTIVE_ONLY |      \
>> +							UB960_ITER_FPD4_ONLY))
>