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



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ