Message-ID: <adc04ee0-4a75-71a5-ef67-a2851265e6e9@amd.com>
Date:   Mon, 10 Apr 2023 15:59:30 -0700
From:   Shannon Nelson <shannon.nelson@....com>
To:     Simon Horman <simon.horman@...igine.com>
Cc:     brett.creeley@....com, davem@...emloft.net, netdev@...r.kernel.org,
        kuba@...nel.org, drivers@...sando.io, leon@...nel.org,
        jiri@...nulli.us
Subject: Re: [PATCH v9 net-next 07/14] pds_core: add FW update feature to
 devlink

On 4/10/23 8:44 AM, Simon Horman wrote:
> 
> On Thu, Apr 06, 2023 at 04:41:36PM -0700, Shannon Nelson wrote:
>> Add in the support for doing firmware updates.  Of the two
>> main banks available, a and b, this updates the one not in
>> use and then selects it for the next boot.
>>
>> Example:
>>      devlink dev flash pci/0000:b2:00.0 \
>>            file pensando/dsc_fw_1.63.0-22.tar
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
> 
> Hi Shannon,
> 
> some minor feedback from my side.

Thanks, I'll take care of these.
sln


> 
> ...
> 
>> diff --git a/Documentation/networking/device_drivers/ethernet/amd/pds_core.rst b/Documentation/networking/device_drivers/ethernet/amd/pds_core.rst
>> index 265d948a8c02..6faf46390f5f 100644
>> --- a/Documentation/networking/device_drivers/ethernet/amd/pds_core.rst
>> +++ b/Documentation/networking/device_drivers/ethernet/amd/pds_core.rst
>> @@ -73,6 +73,16 @@ The ``pds_core`` driver reports the following versions
>>        - fixed
>>        - The revision of the ASIC for this device
>>
>> +Firmware Management
>> +===================
>> +
>> +The ``flash`` command can update a the DSC firmware.  The downloaded firmware
>> +will be saved into either of firmware bank 1 or bank 2, whichever is not
>> +currrently in use, and that bank will used for the next boot::
> 
> nit: s/currrently/currently/



> 
> ...
> 
>> diff --git a/drivers/net/ethernet/amd/pds_core/fw.c b/drivers/net/ethernet/amd/pds_core/fw.c
> 
> ...
> 
>> +int pdsc_firmware_update(struct pdsc *pdsc, const struct firmware *fw,
>> +                      struct netlink_ext_ack *extack)
>> +{
>> +     u32 buf_sz, copy_sz, offset;
>> +     struct devlink *dl;
>> +     int next_interval;
>> +     u64 data_addr;
>> +     int err = 0;
>> +     u8 fw_slot;
>> +
>> +     dev_info(pdsc->dev, "Installing firmware\n");
>> +
>> +     dl = priv_to_devlink(pdsc);
>> +     devlink_flash_update_status_notify(dl, "Preparing to flash",
>> +                                        NULL, 0, 0);
>> +
>> +     buf_sz = sizeof(pdsc->cmd_regs->data);
>> +
>> +     dev_dbg(pdsc->dev,
>> +             "downloading firmware - size %d part_sz %d nparts %lu\n",
>> +             (int)fw->size, buf_sz, DIV_ROUND_UP(fw->size, buf_sz));
>> +
>> +     offset = 0;
>> +     next_interval = 0;
>> +     data_addr = offsetof(struct pds_core_dev_cmd_regs, data);
>> +     while (offset < fw->size) {
>> +             if (offset >= next_interval) {
>> +                     devlink_flash_update_status_notify(dl, "Downloading",
>> +                                                        NULL, offset,
>> +                                                        fw->size);
>> +                     next_interval = offset +
>> +                                     (fw->size / PDSC_FW_INTERVAL_FRACTION);
>> +             }
>> +
>> +             copy_sz = min_t(unsigned int, buf_sz, fw->size - offset);
>> +             mutex_lock(&pdsc->devcmd_lock);
>> +             memcpy_toio(&pdsc->cmd_regs->data, fw->data + offset, copy_sz);
>> +             err = pdsc_devcmd_fw_download_locked(pdsc, data_addr,
>> +                                                  offset, copy_sz);
>> +             mutex_unlock(&pdsc->devcmd_lock);
>> +             if (err) {
>> +                     dev_err(pdsc->dev,
>> +                             "download failed offset 0x%x addr 0x%llx len 0x%x: %pe\n",
>> +                             offset, data_addr, copy_sz, ERR_PTR(err));
>> +                     NL_SET_ERR_MSG_MOD(extack, "Segment download failed");
>> +                     goto err_out;
>> +             }
>> +             offset += copy_sz;
>> +     }
>> +     devlink_flash_update_status_notify(dl, "Downloading", NULL,
>> +                                        fw->size, fw->size);
>> +
>> +     devlink_flash_update_timeout_notify(dl, "Installing", NULL,
>> +                                         PDSC_FW_INSTALL_TIMEOUT);
>> +
>> +     fw_slot = pdsc_devcmd_fw_install(pdsc);
>> +     if (fw_slot < 0) {
> 
> The type of fs_slot is u8.
> But the return type of pdsc_devcmd_fw_install is int,
> (I think) it can return negative error values,
> and that case is checked on the line above.
> 
> Perhaps the type of fw_slot should be int?
> 
> Flagged by Coccinelle as:
> 
> drivers/net/ethernet/amd/pds_core/fw.c:154:5-12: WARNING: Unsigned expression compared with zero: fw_slot < 0
> 
> And Smatch as:
> 
> drivers/net/ethernet/amd/pds_core/fw.c:154 pdsc_firmware_update() warn: impossible condition '(fw_slot < 0) => (0-255 < 0)'
> 
>> +             err = fw_slot;
>> +             dev_err(pdsc->dev, "install failed: %pe\n", ERR_PTR(err));
>> +             NL_SET_ERR_MSG_MOD(extack, "Failed to start firmware install");
>> +             goto err_out;
>> +     }
> 
> ...
> 
> --
> You received this message because you are subscribed to the Google Groups "Pensando Drivers" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to drivers+unsubscribe@...sando.io.
> To view this discussion on the web visit https://groups.google.com/a/pensando.io/d/msgid/drivers/ZDQuxBlfH5foSEFA%40corigine.com.

Powered by blists - more mailing lists