Re: [PATCH v2 net-next 04/12] net: aquantia: add PTP rings infrastructure

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

Hi Andrew,

>>  
>> +	aq_ptp_ring_deinit(self);
>>  	aq_ptp_unregister(self);
>> +	aq_ptp_ring_free(self);
>>  	aq_ptp_free(self);
> 
> Is this order safe? Seems like you should first unregister, and then
> deinit?

Will double check, probably you are right.


>> +	err = aq_ring_init(&aq_ptp->hwts_rx);
>> +	if (err < 0)
>> +		goto err_exit;
>> +	err = aq_nic->aq_hw_ops->hw_ring_rx_init(aq_nic->aq_hw,
>> +						 &aq_ptp->hwts_rx,
>> +						 &aq_ptp->ptp_ring_param);
>> +
>> +err_exit:
>> +	return err;
> 
> Maybe there should be some undoing going on here. If you filled the rx
> ring, do you need to empty it on error?

Right, ptp_rx should be cleaned here.


>> +	err = aq_nic->aq_hw_ops->hw_ring_rx_start(aq_nic->aq_hw,
>> +						  &aq_ptp->hwts_rx);
>> +	if (err < 0)
>> +		goto err_exit;
>> +
>> +err_exit:
> 
> Do you need to stop the rings which started, before the error
> happened?

The failures there are only theoretical (they only do check if the device
is still on PCI bus).

err result will propagate and cause overall NIC object to be failed on open,
eventually aq_nic_deinit will be called and HW will endup in disabled state.

Thus, believe we only have to care not to leak memory, like in above ptp_rx case.


>> +	memset(self, 0, sizeof(*self));
>> +
>> +	self->aq_nic = aq_nic;
>> +	self->idx = idx;
>> +	self->size = size;
>> +	self->dx_size = dx_size;
> 
> More self. Why not ring? I suppose because the rest of this file is
> using the unhelpful self?

Right. I think I'll make a separate patchset to fix this naming,
to not to mixup things.

>> +err_exit:
>> +	if (err < 0) {
>> +		aq_ring_free(self);
>> +		return NULL;
> 
> return ERR_PTR(err)?
> 
>> +	}
>> +
>> +	return self;
> 
> And this code structure seem odd. Why not.

The only cause of error on these *_alloc() functions is ENOMEM.
So think I'll just simplify this, remove all `err` fields and leave
NULL as a fail indicator.

Regards,
  Igor