netdev - Re: [PATCH v2 net-next 2/2] ionic: add devlink firmware update

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <c18aca84-7cd4-64be-1222-2c36c795f024@pensando.io>
Date:   Sat, 5 Sep 2020 14:47:58 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH v2 net-next 2/2] ionic: add devlink firmware update

On 9/5/20 12:52 PM, Jakub Kicinski wrote:
> On Thu,  3 Sep 2020 17:05:34 -0700 Shannon Nelson wrote:
>> +	devlink_flash_update_status_notify(dl, "Downloading", NULL, 0, fw->size);
>> +	offset = 0;
>> +	next_interval = fw->size / IONIC_FW_INTERVAL_FRACTION;
>> +	while (offset < fw->size) {
>> +		copy_sz = min_t(unsigned int, buf_sz, fw->size - offset);
>> +		mutex_lock(&ionic->dev_cmd_lock);
>> +		memcpy_toio(&idev->dev_cmd_regs->data, fw->data + offset, copy_sz);
>> +		ionic_dev_cmd_firmware_download(idev,
>> +						offsetof(union ionic_dev_cmd_regs, data),
>> +						offset, copy_sz);
>> +		err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
>> +		mutex_unlock(&ionic->dev_cmd_lock);
>> +		if (err) {
>> +			netdev_err(netdev,
>> +				   "download failed offset 0x%x addr 0x%lx len 0x%x\n",
>> +				   offset, offsetof(union ionic_dev_cmd_regs, data),
>> +				   copy_sz);
>> +			NL_SET_ERR_MSG_MOD(extack, "Segment download failed");
>> +			goto err_out;
>> +		}
>> +		offset += copy_sz;
>> +
>> +		if (offset > next_interval) {
>> +			devlink_flash_update_status_notify(dl, "Downloading",
>> +							   NULL, offset, fw->size);
>> +			next_interval = offset + (fw->size / IONIC_FW_INTERVAL_FRACTION);
>> +		}
>> +	}
>> +	devlink_flash_update_status_notify(dl, "Downloading", NULL, 1, 1);
> This is quite awkward. You send a notification with a different size,
> and potentially an unnecessary one if last iteration of the loop
> triggered offset > next_interval.
>
> Please just add || offset == fw->size to the condition at the end of
> the loop and it will always trigger, with the correct length.

Or maybe make that last one look like
     devlink_flash_update_status_notify(dl, "Downloading", NULL, 
fw->size, fw->size);
to be less awkward and to keep the style of using a final status_notify 
at the end of the block, as done in the Install and Select blocks 
further along?

sln