netdev - Re: [PATCH net-next 2/2] qlcnic: Change CDRP function

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: <AF200FAC-89A4-4A33-A96F-B9629D1FAD60@qlogic.com>
Date:	Tue, 13 Sep 2011 14:15:10 -0700
From:	Anirban Chakraborty <anirban.chakraborty@...gic.com>
To:	Joe Perches <joe@...ches.com>
CC:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Dept_NX_Linux_NIC_Driver <Dept_NX_Linux_NIC_Driver@...gic.com>
Subject: Re: [PATCH net-next 2/2] qlcnic: Change CDRP function


On Sep 13, 2011, at 1:56 PM, Joe Perches wrote:

>     On Tue, 2011-09-13 at 13:28 -0700, Anirban Chakraborty wrote:
>> On Sep 13, 2011, at 11:22 AM, Joe Perches wrote:
>>> On Tue, 2011-09-13 at 11:06 -0700, Anirban Chakraborty wrote:
>>>> Argument list to CDRP function has become unmanageably long. Fix it by properly
>>>> declaring a struct that encompasses all the input and output parameters.
>>> I don't think this is better.
>>> I think you've overloaded the issue_cmd function and should
>>> use separate issue_cmd_<type> functions instead.
>> I did not overload issue_cmd(), instead I changed the function signature. 
>> -u32
>> -qlcnic_issue_cmd(struct qlcnic_adapter *adapter,
>> -       u32 pci_fn, u32 version, u32 arg1, u32 arg2, u32 arg3, u32 cmd,
>> -               u32 *rd_args[3])
>> +void
>> +qlcnic_issue_cmd(struct qlcnic_adapter *adapter, struct qlcnic_cmd_args *cmd)
>> Instead of having a long list of arguments, I put them into a struct and passed it around.
> 
> Yes, I saw that.
> 
> I think that if restructuring is done, perhaps there are
> better alternatives to bundling all the arguments into a
> single struct.
> 
> I think it might be more sensible to have multiple
> issue_cmd_<cmd>(adapter, [appropriate_args]) functions
> instead so that the arguments and returns are appropriate
> for the cmd performed.
> 
> or maybe use
> 
> issue_cmd(adapter, cmd, struct)
> 
> so you make cmd explicit and it's a bit easier to read
> if you really must use some struct where cmd doesn't
> use all the elements of struct.

The usage of issue_cmd() here is to send a message to the FW. In some cases, the caller
of the function may want to read additional data right after the FW cmd is executed and in some
cases it doesn't.  As you mentioned, one way to do is to use two different issue_cmd()s. However,
we find it easier to use a single issue_cmd(), where the caller indicates whether it needs additional
data by setting the parameters in the argument. 

Thank you for your feedback.

-Anirban


--
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