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

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <14148d19-5869-f510-2a08-e3d69ad25575@aquantia.com>
Date:   Tue, 15 Oct 2019 09:02:38 +0000
From:   Igor Russkikh <Igor.Russkikh@...antia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        Egor Pomozov <Egor.Pomozov@...antia.com>,
        Dmitry Bezrukov <Dmitry.Bezrukov@...antia.com>,
        Simon Edelhaus <sedelhaus@...vell.com>,
        Sergey Samoilenko <Sergey.Samoilenko@...antia.com>
Subject: Re: [PATCH v2 net-next 04/12] net: aquantia: add PTP rings
 infrastructure

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