Re: [PATCH net-next 3/5] dpll: zl3073x: Add firmware loading functionality

[Ivan Vecera <ivecera@redhat.com>]

On 26. 07. 25 10:33 odp., Simon Horman wrote:
> On Fri, Jul 25, 2025 at 05:41:34PM +0200, Ivan Vecera wrote:
>> Add functionality for loading firmware files provided by the vendor
>> to be flashed into the device's internal flash memory. The firmware
>> consists of several components, such as the firmware executable itself,
>> chip-specific customizations, and configuration files.
>>
>> The firmware file contains at least a flash utility, which is executed
>> on the device side, and one or more flashable components. Each component
>> has its own specific properties, such as the address where it should be
>> loaded during flashing, one or more destination flash pages, and
>> the flashing method that should be used.
>>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> 
> Hi Ivan,
> 
> Some minor feedback from my side.
> 
> ...
> 
>> diff --git a/drivers/dpll/zl3073x/fw.c b/drivers/dpll/zl3073x/fw.c
> 
> ...

Hi Simon,

> 
>> +/* Santity check */
> 
> Sanity

Will fix in v2.

>> +static_assert(ARRAY_SIZE(component_info) == ZL_FW_NUM_COMPONENTS);
> 
> ...
> 
>> +int zl3073x_fw_flash(struct zl3073x_dev *zldev, struct zl3073x_fw *zlfw,
>> +		     struct netlink_ext_ack *extack)
>> +{
>> +	int i, rc;
>> +
>> +	for (i = 0; i < ZL_FW_NUM_COMPONENTS; i++) {
>> +		if (!zlfw->component[i])
>> +			continue; /* Component is not present */
>> +
>> +		rc = zl3073x_fw_component_flash(zldev, zlfw->component[i],
>> +						extack);
>> +		if (rc)
>> +			break;
>> +	}
> 
> Perhaps it cannot happen in practice.
> But Smatch warns that rc may be used uninitialised below.
> And that does seem theoretically possible if all
> iterations of the loop above hit the "continue" path.

Yes, it should not happen as at least one component has to be present
but for sure I will init rc in the next iteration.

Thanks for review,
Ivan

> 
>> +
>> +	return rc;
>> +}
> 
> ...
>