netdev - Re: [PATCH 6/9] ethernet: cavium: Replace deprecated PCI functions

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]

Message-ID: <ZsRwsDWQT6dmwC1p@smile.fi.intel.com>
Date: Tue, 20 Aug 2024 13:32:16 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Philipp Stanner <pstanner@...hat.com>
Cc: onathan Corbet <corbet@....net>, Jens Axboe <axboe@...nel.dk>,
	Wu Hao <hao.wu@...el.com>, Tom Rix <trix@...hat.com>,
	Moritz Fischer <mdf@...nel.org>, Xu Yilun <yilun.xu@...el.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Alvaro Karsz <alvaro.karsz@...id-run.com>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Jason Wang <jasowang@...hat.com>,
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
	Eugenio Pérez <eperezma@...hat.com>,
	Richard Cochran <richardcochran@...il.com>,
	Mark Brown <broonie@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	Hannes Reinecke <hare@...e.de>, Damien Le Moal <dlemoal@...nel.org>,
	Chaitanya Kulkarni <kch@...dia.com>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-block@...r.kernel.org, linux-fpga@...r.kernel.org,
	linux-gpio@...r.kernel.org, netdev@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, linux-pci@...r.kernel.org,
	virtualization@...ts.linux.dev
Subject: Re: [PATCH 6/9] ethernet: cavium: Replace deprecated PCI functions

On Tue, Aug 20, 2024 at 09:40:09AM +0200, Philipp Stanner wrote:
> On Mon, 2024-08-19 at 21:23 +0300, Andy Shevchenko wrote:
> > On Mon, Aug 19, 2024 at 06:51:46PM +0200, Philipp Stanner wrote:

...

> > cavium_ptp_probe()
> > 
> > > -	pcim_iounmap_regions(pdev, 1 << PCI_PTP_BAR_NO);
> > > +	pcim_iounmap_region(pdev, PCI_PTP_BAR_NO);
> > >  
> > >  error_free:
> > >  	devm_kfree(dev, clock);
> > 
> > Both are questionable. Why do we need either of them?
> 
> You seem to criticize my pcim_iounmap_region() etc. in other unwind
> paths, too.

Yes, having devm/pcim/etc_m in the clean up / error paths seems at bare minimum
confusing, or reveals wrong use of them or even misunderstanding the concept...

And it's not your fault, it was already in those drivers like that...

> I think your criticism is often justified. This driver
> here, however, was the one which made me suspicious and hesitate and
> removing those calls; because of the code below:
> 
> 
> 	pcim_iounmap_region(pdev, PCI_PTP_BAR_NO);
> 
> error_free:
> 	devm_kfree(dev, clock);
> 
> error:
> 	/* For `cavium_ptp_get()` we need to differentiate between the case
> 	 * when the core has not tried to probe this device and the case when
> 	 * the probe failed.  In the later case we pretend that the
> 	 * initialization was successful and keep the error in
> 	 * `dev->driver_data`.
> 	 */
> 	pci_set_drvdata(pdev, ERR_PTR(err));
> 	return 0;
> }
> 
> So in case of an error they return 0 and do... stuff.
> 
> I don't want to touch that without someone who maintains (and, ideally,
> understands) the code details what's going on here.

Thanks for elaboration, indeed it was not enough context to see the full
picture. This seems like an ugly hack that has to be addressed at some point.
But again, not your fault.

-- 
With Best Regards,
Andy Shevchenko