netdev - Re: [PATCHv2 net-next-2.6] ethtool: Added support for FW dump

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: <8EE73B82-F30A-41F7-B220-33362F0E45C4@qlogic.com>
Date:	Tue, 10 May 2011 13:22:55 -0700
From:	Anirban Chakraborty <anirban.chakraborty@...gic.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
CC:	netdev <netdev@...r.kernel.org>, David Miller <davem@...emloft.net>
Subject: Re: [PATCHv2 net-next-2.6] ethtool: Added support for FW dump


On May 10, 2011, at 11:29 AM, Ben Hutchings wrote:

> On Mon, 2011-05-09 at 18:02 -0700, Anirban Chakraborty wrote:
>> Added code to take FW dump via ethtool. A pair of set and get functions are
>> added to configure dump level and fetch dump data from the driver respectively.
> 
> I don't understand why you are combining get-flags and get-data in the
> driver interface.  I suggested that you could use a single option for
> these in the ethtool *utility*, but combining them in the driver
> interface just seems to complicate the implementation of the ethtool
> core code and the driver.

I was under the impression that you wanted to collapse get data and flag into one operation,
both in ethtool and in the driver. Will change it in next version.

> 
>> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@...gic.com>
>> ---
>> include/linux/ethtool.h |   31 +++++++++++++++++++++++
>> net/core/ethtool.c      |   62 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 93 insertions(+), 0 deletions(-)
>> 
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index bd0b50b..f2cd7e1 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -601,6 +601,31 @@ struct ethtool_flash {
>> 	char	data[ETHTOOL_FLASH_MAX_FILENAME];
>> };
>> 
>> +/**
>> + * struct ethtool_dump - used for retrieving, setting device dump
> 
> Missing description of @cmd.
> 
>> + * @type: type of operation, get dump settings or data
>> + * @version: FW version of the dump
>> + * @flag: flag for dump setting
>> + * @len: length of dump data
>> + * @data: data collected for this command
> 
> The kernel-doc needs to describe when the fields are valid - i.e. which
> commands use them as input and/or output.

Will fix.

> <snip>
>> 
>> +
>> +	data = vzalloc(dump.len);
>> +	if (!data)
>> +		return -ENOMEM;
>> +	ret = ops->get_dump(dev, &dump, data);
>> +	if (ret) {
>> +		ret = -EFAULT;
> 
> There is no reason to change ret here.  Didn't I already raise this in
> version 1?

Yes you did. Although I fixed others, but some how I missed this one.
> 
>> +		goto out;
>> +	}
>> +	useraddr += offsetof(struct ethtool_dump, data);
>> +	if (copy_to_user(useraddr, data, dump.len))
> 
> The copied length should be the *minimum* of the user's buffer length
> and the actual dump length.
Will fix it.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html