netdev - Re: [PATCH v3] ethernet/arc/arc_emac

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]

Date:	Thu, 13 Jun 2013 23:50:41 +0300
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>
Cc:	netdev <netdev@...r.kernel.org>,
	Francois Romieu <romieu@...zoreil.com>,
	Joe Perches <joe@...ches.com>,
	Vineet Gupta <Vineet.Gupta1@...opsys.com>,
	Mischa Jonker <Mischa.Jonker@...opsys.com>,
	Arnd Bergmann <arnd@...db.de>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	"David S. Miller" <davem@...emloft.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Devicetree Discuss <devicetree-discuss@...ts.ozlabs.org>
Subject: Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver

On Thu, Jun 13, 2013 at 11:25 PM, Alexey Brodkin
<Alexey.Brodkin@...opsys.com> wrote:
> On 06/13/2013 10:25 PM, Andy Shevchenko wrote:
>> On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
>> <Alexey.Brodkin@...opsys.com> wrote:
>>> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
>>> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
>>> ARCAngel4/ML50x.

>>> +++ b/drivers/net/ethernet/arc/arc_emac.h

> And what about function names? Do you think it worth to shorten them too
> since most of them aren't visible outside (static).

It's better to keep them in their namespace. So, leave them as is.

>>> +struct arc_emac_priv {

>>> +};

>> It seems you missed my comments against the names of the members. Can
>> you address them or comment why not?
>
> You mean to add description in kerneldoc format for all the fields in
> structures?

Not only that one. About member names as well.

> Well while in general it could be "a proper way" of documenting sources
> I found it not that convenient especially in case of really long structures.

> In my case "arc_emac_priv" structure has 21 members, so right before
> structure itself there will be another at least 21 line of comments.

Not an argument, you understand.

> Moreover: "The kernel-doc function comments describe each parameter to
> the function, in order, with the @name lines."

> While I don't think that each and every member needs description.

Describe them in couple of words.

> At
> least some pairs like Tx/Rx I believe may share the only comment saying
> "Pointers to BD rings - CPU side".

What BD means? May be it worth to describe as well?

> Also I barely can find an example of strict usage of kernel-doc format
> for data structures in drivers nearby.
>
> For example take a look at STMMAC - drivers/net/ethernet/stmicro/stmmac/
> Lots of structures defined, non with kernel-doc description.

Again, not an excuse :-)

> Still you think the only way to go is to add kernel-doc description then
> I'll add it ASAP, might be it will be a good example for other developers.

Right.

>>> +++ b/drivers/net/ethernet/arc/arc_emac_main.c
>>

>>> +static int arc_emac_poll(struct napi_struct *napi, int budget)
>>> +{

>>> +       unsigned int i, loop, work_done = 0;

>>> +       for (loop = 0; loop < RX_BD_NUM; loop++) {

>>> +               work_done++;
>>> +               if (work_done >= budget)
>>> +                       break;
>>
>> Those three could easily go to the for () on the top of this function.
>
> Correct. It should be like this on top of the "arc_emac_poll":
> ====
>                 if (work_done >= budget)
>                         break;
>                 work_done++;
> ====

I meant something like
for (loop = 0; loop < RX_BD_NUM && work_done < budget; loop++, work_done++)

>>> +static int arc_emac_open(struct net_device *ndev)
>>> +{
>>
>>> +       /* Set Poll rate so that it polls every 1 ms */
>>> +       arc_reg_set(priv, R_POLLRATE,
>>> +                    priv->clock_frequency / 1000000);
>>
>> I don't understand how you end up with 1ms here. 1000000 is just a
>> magic number, clock_frequency generally is an arbitrary value.

I meant how do you guarantee this is 1ms? What if clock_frequency is not 100MHz?

--
With Best Regards,
Andy Shevchenko
--
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