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

[Igor Russkikh <Igor.Russkikh@aquantia.com>]

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