netdev - RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

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: <005a01d98c8b$e48d2b60$ada78220$@trustnetic.com>
Date: Mon, 22 May 2023 17:00:44 +0800
From: Jiawen Wu <jiawenwu@...stnetic.com>
To: "'Andrew Lunn'" <andrew@...n.ch>,
	"'Michael Walle'" <michael@...le.cc>
Cc: "'Andy Shevchenko'" <andy.shevchenko@...il.com>,
	<netdev@...r.kernel.org>,
	<jarkko.nikula@...ux.intel.com>,
	<andriy.shevchenko@...ux.intel.com>,
	<mika.westerberg@...ux.intel.com>,
	<jsd@...ihalf.com>,
	<Jose.Abreu@...opsys.com>,
	<hkallweit1@...il.com>,
	<linux@...linux.org.uk>,
	<linux-i2c@...r.kernel.org>,
	<linux-gpio@...r.kernel.org>,
	<mengyuanlou@...-swift.com>
Subject: RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

On Friday, May 19, 2023 9:13 PM, Andrew Lunn wrote:
> > I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK).
> > It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO
> > interrupt is determined, GPIO_INT_STATUS register should be read to determine
> > which GPIO line has changed state.
> 
> So you have another interrupt controller above the GPIO interrupt
> controller. regmap-gpio is pushing you towards describing this
> interrupt controller as a Linux interrupt controller.
> 
> When you look at drivers handling interrupts, most leaf interrupt
> controllers are not described as Linux interrupt controllers. The
> driver interrupt handler reads the interrupt status register and
> internally dispatches to the needed handler. This works well when
> everything is internal to one driver.
> 
> However, here, you have two drivers involved, your MAC driver and a
> GPIO driver instantiated by the MAC driver. So i think you are going
> to need to described the MAC interrupt controller as a Linux interrupt
> controller.
> 
> Take a look at the mv88e6xxx driver, which does this. It has two
> interrupt controller embedded within it, and they are chained.

Now I add two interrupt controllers, the first one for the MAC interrupt,
and the second one for regmap-gpio. In the second adding flow,

	irq = irq_find_mapping(txgbe->misc.domain, TXGBE_PX_MISC_GPIO_OFFSET);
	err = regmap_add_irq_chip_fwnode(fwnode, regmap, irq, 0, 0,
					 chip, &chip_data);

and then,

	config.irq_domain = regmap_irq_get_domain(chip_data);
	gpio_regmap = gpio_regmap_register(&config);

"txgbe->misc.domain" is the MAC interrupt domain. I think this flow should
be correct, but still failed to get gpio_irq from gpio_desc with err -517.

And I still have doubts about what I said earlier:
https://lore.kernel.org/netdev/20230515063200.301026-1-jiawenwu@trustnetic.com/T/#me1be68e1a1e44426ecc0dd8edf0f6b224e50630d

There really is nothing wrong with gpiochip_to_irq()??

> > > If you are getting errors when removing the driver it means you are
> > > missing some level of undoing what us done in probe. Are you sure
> > > regmap_del_irq_chip() is being called on unload?
> >
> > I used devm_* all when I registered them.
> 
> Look at the ordering. Is regmap_del_irq_chip() being called too late?
> I've had problems like this with the mv88e6xxx driver and its
> interrupt controllers. I ended up not using devm_ so i had full
> control over the order things got undone. In that case, the external
> devices was PHYs, with the PHY interrupt being inside the Ethernet
> switch, which i exposed using a Linux interrupt controller.

I use no devm_ functions to add regmap irq chip, register gpio regmap,
and call their del/unregister functions at the position corresponding to
release. irq_domain_remove() call trace still exist.

[  104.553182] Call Trace:
[  104.553184]  <TASK>
[  104.553185]  irq_domain_remove+0x2b/0xe0
[  104.553190]  regmap_del_irq_chip.part.0+0x8a/0x160
[  104.553196]  txgbe_remove_phy+0x57/0x80 [txgbe]
[  104.553201]  txgbe_remove+0x2a/0x90 [txgbe]
[  104.553205]  pci_device_remove+0x36/0xa0
[  104.553208]  device_release_driver_internal+0xaa/0x140
[  104.553213]  driver_detach+0x44/0x90
[  104.553215]  bus_remove_driver+0x69/0xf0
[  104.553217]  pci_unregister_driver+0x29/0xb0
[  104.553220]  __x64_sys_delete_module+0x145/0x240
[  104.553223]  ? exit_to_user_mode_prepare+0x3c/0x1a0
[  104.553226]  do_syscall_64+0x3b/0x90
[  104.553230]  entry_SYSCALL_64_after_hwframe+0x72/0xdc