On Tue, Aug 20, 2024 at 10:13:40AM +0200, Philipp Stanner wrote:

On Mon, 2024-08-19 at 21:34 +0300, Andy Shevchenko wrote:

quoted

On Mon, Aug 19, 2024 at 08:19:28PM +0200, Christophe JAILLET wrote:

quoted

Le 19/08/2024 à 18:51, Philipp Stanner a écrit :

...

quoted

quoted

Unrelated to the patch, but is is safe to have 'name' be on the
stack?

pcim_iomap_region()
--> __pcim_request_region()
--> __pcim_request_region_range()
--> request_region() or __request_mem_region()
--> __request_region()
--> __request_region_locked()
--> res->name = name;

So an address on the stack ends in the 'name' field of a "struct
resource".

According to a few grep, it looks really unusual.

I don't know if it is used, but it looks strange to me.

It might be used when printing /proc/iomem, but I don't remember by
heart.

quoted

If it is an issue, it was apparently already there before this
patch.

This series seems to reveal a lot of issues with the probe/remove in
many
drivers. I think it's better to make fixes of them before this series
for
the sake of easier backporting.

Just so we're in sync:
I think the only real bug here so far is the one found by Christophe.

The usages of pci_disable_device(), pcim_iounmap_regions() and the like
in remove() and error unwind paths are not elegant and make devres kind
of useless – but they are not bugs. So I wouldn't backport them.

Agree.

quoted

If here is a problem, the devm_kasprintf() should be used.

-- 
With Best Regards,
Andy Shevchenko