netdev - Re: [PATCH net-next 01/11] net: aquantia: PTP skeleton declarations and callbacks

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

Message-ID: <bb528d8c-575b-2d0c-6105-f0bd1f4d5d2c@aquantia.com>
Date:   Wed, 11 Sep 2019 11:41:01 +0000
From:   Igor Russkikh <Igor.Russkikh@...antia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Egor Pomozov <Egor.Pomozov@...antia.com>,
        Sergey Samoilenko <Sergey.Samoilenko@...antia.com>,
        Dmitry Bezrukov <Dmitry.Bezrukov@...antia.com>
Subject: Re: [PATCH net-next 01/11] net: aquantia: PTP skeleton declarations
 and callbacks


Hi Andrew,

>> +	struct ptp_clock *clock;
>> +	struct aq_ptp_s *self;
> 
> I find the use of self in this code quite confusing. It does not
> appear to have a clear meaning. It can be a aq_ring_s, aq_nic_c,
> aq_hw_s, aq_vec_s.
> 
> Looking at this code i always have to figure out what self is. Could
> you not just use struct aq_ptp_s aq_ptp consistently in the code?

Agreed,

>> +
>> +	self = kzalloc(sizeof(*self), GFP_KERNEL);
> 
> Using devm_kzalloc() will make your clean up easier.

>> +
>> +	kfree(self);
> 
> kfree() is happy to take a NULL pointer. But this could all go away
> with devm_kzalloc().

You are probably right, that'll be easier,


>> +static int hw_atl_b0_adj_sys_clock(struct aq_hw_s *self, s64 delta)
>> +{
>> +	ptp_clk_offset += delta;
>> +
>> +	return 0;
>> +}
> 
> Does this work when i have a box with 42 NICs in it? Does not each NIC
> need its own clock offset? Just seeing code like this causes alarm
> bells. So if it is correct, i would expect some sort of comment to
> prevent those alarm bells.

No comment is needed, that is obviously a per-device variable,

Thanks for catching this!

Will fix that, as well as kbuild robot discovered issues.

Regards,
  Igor