diff mbox series

[v2,1/1] serial: core: Fix serial_base_match() after fixing controller port name

Message ID 20230803071034.25571-1-tony@atomide.com
State New
Headers
Series [v2,1/1] serial: core: Fix serial_base_match() after fixing controller port name |

Commit Message

Tony Lindgren Aug. 3, 2023, 7:10 a.m. UTC 
  While fixing DEVNAME to be more usable, I broke serial_base_match() as the
ctrl and port prefix for device names seemed unnecessary.

The prefixes are still needed by serial_base_match() to probe the serial
base controller port, and serial tx is now broken.

Let's fix the issue by checking against dev->type and drv->name instead
of the prefixes that are no longer in the DEVNAME.

Fixes: 1ef2c2df1199 ("serial: core: Fix serial core controller port name to show controller id")
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202308021529.35b3ad6c-oliver.sang@intel.com
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Changes since v1:
- Leave out magic numbers and use str_has_prefix() as suggested by Andy
  and Greg

- Improve patch description and add a link for Closes tag as suggested
  by Jiri

- Check the name against device_type name since we have it and leave
  out the changes to try to define names in the header because of the
  issues noted by Jiri

- Leave out Tested-by from Mark and Anders as the patch changed

---
 drivers/tty/serial/serial_base_bus.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Jiri Slaby Aug. 3, 2023, 7:26 a.m. UTC | #1 
On 03. 08. 23, 9:10, Tony Lindgren wrote:
> While fixing DEVNAME to be more usable, I broke serial_base_match() as the
> ctrl and port prefix for device names seemed unnecessary.
> 
> The prefixes are still needed by serial_base_match() to probe the serial
> base controller port, and serial tx is now broken.
> 
> Let's fix the issue by checking against dev->type and drv->name instead
> of the prefixes that are no longer in the DEVNAME.
> 
> Fixes: 1ef2c2df1199 ("serial: core: Fix serial core controller port name to show controller id")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202308021529.35b3ad6c-oliver.sang@intel.com
> Signed-off-by: Tony Lindgren <tony@atomide.com>

A lot better:

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
Greg KH Aug. 3, 2023, 7:50 a.m. UTC | #2 
On Thu, Aug 03, 2023 at 10:10:32AM +0300, Tony Lindgren wrote:
> While fixing DEVNAME to be more usable, I broke serial_base_match() as the
> ctrl and port prefix for device names seemed unnecessary.
> 
> The prefixes are still needed by serial_base_match() to probe the serial
> base controller port, and serial tx is now broken.
> 
> Let's fix the issue by checking against dev->type and drv->name instead
> of the prefixes that are no longer in the DEVNAME.
> 
> Fixes: 1ef2c2df1199 ("serial: core: Fix serial core controller port name to show controller id")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202308021529.35b3ad6c-oliver.sang@intel.com
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 
> Changes since v1:
> - Leave out magic numbers and use str_has_prefix() as suggested by Andy
>   and Greg
> 
> - Improve patch description and add a link for Closes tag as suggested
>   by Jiri
> 
> - Check the name against device_type name since we have it and leave
>   out the changes to try to define names in the header because of the
>   issues noted by Jiri
> 
> - Leave out Tested-by from Mark and Anders as the patch changed

Thanks for this, now queued up.

greg k-h
Conor Dooley Aug. 3, 2023, 8:52 a.m. UTC | #3 
On Thu, Aug 03, 2023 at 09:50:03AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 03, 2023 at 10:10:32AM +0300, Tony Lindgren wrote:
> > While fixing DEVNAME to be more usable, I broke serial_base_match() as the
> > ctrl and port prefix for device names seemed unnecessary.
> > 
> > The prefixes are still needed by serial_base_match() to probe the serial
> > base controller port, and serial tx is now broken.
> > 
> > Let's fix the issue by checking against dev->type and drv->name instead
> > of the prefixes that are no longer in the DEVNAME.
> > 
> > Fixes: 1ef2c2df1199 ("serial: core: Fix serial core controller port name to show controller id")
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Closes: https://lore.kernel.org/oe-lkp/202308021529.35b3ad6c-oliver.sang@intel.com
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> > 
> > Changes since v1:
> > - Leave out magic numbers and use str_has_prefix() as suggested by Andy
> >   and Greg
> > 
> > - Improve patch description and add a link for Closes tag as suggested
> >   by Jiri
> > 
> > - Check the name against device_type name since we have it and leave
> >   out the changes to try to define names in the header because of the
> >   issues noted by Jiri
> > 
> > - Leave out Tested-by from Mark and Anders as the patch changed
> 
> Thanks for this, now queued up.

Seems like I am a bit late, but FWIW this does fix my boot failures in
-next:
Tested-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.
Biju Das Aug. 3, 2023, 3:34 p.m. UTC | #4 
Hi,

> While fixing DEVNAME to be more usable, I broke serial_base_match() as the
> ctrl and port prefix for device names seemed unnecessary.

> The prefixes are still needed by serial_base_match() to probe the serial
> base controller port, and serial tx is now broken.

> Let's fix the issue by checking against dev->type and drv->name instead
> of the prefixes that are no longer in the DEVNAME.

> Fixes: 1ef2c2df1199 ("serial: core: Fix serial core controller port name to show controller id")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202308021529.35b3ad6c-oliver.sang@intel.com
> Signed-off-by: Tony Lindgren <tony@atomide.com>

This patch fixes the boot issue on RZ/G2L SMARC EVK since yesterday.

Tested-by: Biju Das <biju.das.jz@bp.renesas.com>
Guenter Roeck Aug. 4, 2023, 9:42 p.m. UTC | #5 
On Thu, Aug 03, 2023 at 10:10:32AM +0300, Tony Lindgren wrote:
> While fixing DEVNAME to be more usable, I broke serial_base_match() as the
> ctrl and port prefix for device names seemed unnecessary.
> 
> The prefixes are still needed by serial_base_match() to probe the serial
> base controller port, and serial tx is now broken.
> 
> Let's fix the issue by checking against dev->type and drv->name instead
> of the prefixes that are no longer in the DEVNAME.
> 
> Fixes: 1ef2c2df1199 ("serial: core: Fix serial core controller port name to show controller id")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202308021529.35b3ad6c-oliver.sang@intel.com
> Signed-off-by: Tony Lindgren <tony@atomide.com>

With this patch applied, I see the following traceback in the pending-fixes
branch.

Bisect log attached as well. It actually points to commit d962de6ae51f
("serial: core: Fix serial core port id to not use port->line").
Bisect was on mips, but I also see problems on arm, ppc, and sparc.
sparc boot tests show the warning message and then stall until aborted
(which of course may be a different problem).

Guenter

---
printk: console [ttyS0] enabled
sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G                 N 6.5.0-rc4-00350-g514250232787 #1
Stack : ffffffff 801adf80 80e74014 00000004 818e0000 00000000 821ab77c 92b3462d
        81140000 812b9940 81140000 821b0450 81202193 00000001 821ab720 821bc000
        00000000 00000000 8104844c 0000018e 00000001 0000018f 00000000 00000165
        648ce409 00000002 81140000 fff80000 81140000 8104844c 00000001 82a50548
        82a4ac04 00000000 821aba48 00000017 00000000 808398ec 00000000 812c0000
        ...
Call Trace:
[<8010a0e8>] show_stack+0x38/0x118
[<80d92a70>] dump_stack_lvl+0xa4/0xf0
[<803ace54>] sysfs_warn_dup+0x68/0x84
[<803acfd0>] sysfs_create_dir_ns+0xf8/0x110
[<80d4754c>] kobject_add_internal+0xc4/0x2fc
[<80d47db8>] kobject_add+0x64/0xe4
[<8084a824>] device_add+0x110/0x790
[<808357ac>] serial_base_port_add+0x100/0x134
[<80834aa4>] serial_core_register_port+0x88/0x6b0
[<808362bc>] serial8250_register_8250_port+0x398/0x5c8
[<808367fc>] serial8250_probe+0x150/0x1e4
[<80851894>] platform_probe+0x50/0xbc
[<8084eb0c>] really_probe+0xc0/0x378
[<8084efec>] driver_probe_device+0x48/0x110
[<8084f25c>] __driver_attach+0xb0/0x180
[<8084c5c8>] bus_for_each_dev+0x84/0xe8
[<8084ddb4>] bus_add_driver+0x174/0x1fc
[<80850030>] driver_register+0x88/0x15c
[<812425a4>] serial8250_init+0x170/0x1d0
[<80100c28>] do_one_initcall+0x88/0x330
[<81219134>] kernel_init_freeable+0x204/0x2b0
[<80d956a4>] kernel_init+0x24/0x118
[<80102fd8>] ret_from_kernel_thread+0x14/0x1c

kobject: kobject_add_internal failed for serial8250.0:0.0 with -EEXIST, don't try to register things with the same name in the same directory.
serial8250 serial8250.0: unable to register port at index 1 (IO2f8 MEM0 IRQ3): -17
sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G                 N 6.5.0-rc4-00350-g514250232787 #1
Stack : ffffffff 801adf80 80e74014 00000004 818e0000 00000000 821ab77c 92b3462d
        81140000 812b9940 81140000 821b0450 81202193 00000001 821ab720 821bc000
        00000000 00000000 8104844c 000001b1 00000001 000001b2 00000000 00000165
        648ce409 00000002 81140000 fff80000 81140000 8104844c 00000001 82a50548
        82a48804 00000000 821aba48 00000017 00000000 808398ec 00000000 812c0000
        ...
Call Trace:
[<8010a0e8>] show_stack+0x38/0x118
[<80d92a70>] dump_stack_lvl+0xa4/0xf0
[<803ace54>] sysfs_warn_dup+0x68/0x84
[<803acfd0>] sysfs_create_dir_ns+0xf8/0x110
[<80d4754c>] kobject_add_internal+0xc4/0x2fc
[<80d47db8>] kobject_add+0x64/0xe4
[<8084a824>] device_add+0x110/0x790
[<808357ac>] serial_base_port_add+0x100/0x134
[<80834aa4>] serial_core_register_port+0x88/0x6b0
[<808362bc>] serial8250_register_8250_port+0x398/0x5c8
[<808367fc>] serial8250_probe+0x150/0x1e4
[<80851894>] platform_probe+0x50/0xbc
[<8084eb0c>] really_probe+0xc0/0x378
[<8084efec>] driver_probe_device+0x48/0x110
[<8084f25c>] __driver_attach+0xb0/0x180
[<8084c5c8>] bus_for_each_dev+0x84/0xe8
[<8084ddb4>] bus_add_driver+0x174/0x1fc
[<80850030>] driver_register+0x88/0x15c
[<812425a4>] serial8250_init+0x170/0x1d0
[<80100c28>] do_one_initcall+0x88/0x330
[<81219134>] kernel_init_freeable+0x204/0x2b0
[<80d956a4>] kernel_init+0x24/0x118
[<80102fd8>] ret_from_kernel_thread+0x14/0x1c

kobject: kobject_add_internal failed for serial8250.0:0.0 with -EEXIST, don't try to register things with the same name in the same directory.
serial8250 serial8250.0: unable to register port at index 2 (IO0 MEM1f000900 IRQ20): -17

---
# bad: [514250232787bc4c20714949414b314a161120b4] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
# good: [5d0c230f1de8c7515b6567d9afba1f196fb4e2f4] Linux 6.5-rc4
git bisect start 'HEAD' 'v6.5-rc4'
# good: [3678fcb6d3eed56ab276f8211a2fd0cae245ac81] Merge branch 'mm-hotfixes-unstable' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
git bisect good 3678fcb6d3eed56ab276f8211a2fd0cae245ac81
# bad: [7bbae783b7eea8c2b47ec201abd8fbab135c641c] Merge branch 'fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git
git bisect bad 7bbae783b7eea8c2b47ec201abd8fbab135c641c
# good: [3f649ff97541b87652c1c8fe4762dd63d59088d7] Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
git bisect good 3f649ff97541b87652c1c8fe4762dd63d59088d7
# bad: [b0aa91d9eb582c1e1f773516945e125ba01c76bd] Merge branch 'fixes-togreg' of git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
git bisect bad b0aa91d9eb582c1e1f773516945e125ba01c76bd
# good: [8a4629055ef55177b5b63dab1ecce676bd8cccdd] iio: cros_ec: Fix the allocation size for cros_ec_command
git bisect good 8a4629055ef55177b5b63dab1ecce676bd8cccdd
# bad: [7d695d83767cdb4288b101affef6d1d1bcf44d31] serial: core: Fix serial_base_match() after fixing controller port name
git bisect bad 7d695d83767cdb4288b101affef6d1d1bcf44d31
# good: [83c35180abfdfb22f3d7703b0c85ad2d442ed2c5] serial: core: Controller id cannot be negative
git bisect good 83c35180abfdfb22f3d7703b0c85ad2d442ed2c5
# bad: [1ef2c2df11997b8135f34adcf2c200d3b4aacbe9] serial: core: Fix serial core controller port name to show controller id
git bisect bad 1ef2c2df11997b8135f34adcf2c200d3b4aacbe9
# bad: [d962de6ae51f9b76ad736220077cda83084090b1] serial: core: Fix serial core port id to not use port->line
git bisect bad d962de6ae51f9b76ad736220077cda83084090b1
# first bad commit: [d962de6ae51f9b76ad736220077cda83084090b1] serial: core: Fix serial core port id to not use port->line
Tony Lindgren Aug. 5, 2023, 4:49 a.m. UTC | #6 
* Guenter Roeck <linux@roeck-us.net> [230804 21:42]:
> On Thu, Aug 03, 2023 at 10:10:32AM +0300, Tony Lindgren wrote:
> > While fixing DEVNAME to be more usable, I broke serial_base_match() as the
> > ctrl and port prefix for device names seemed unnecessary.
> > 
> > The prefixes are still needed by serial_base_match() to probe the serial
> > base controller port, and serial tx is now broken.
> > 
> > Let's fix the issue by checking against dev->type and drv->name instead
> > of the prefixes that are no longer in the DEVNAME.
> > 
> > Fixes: 1ef2c2df1199 ("serial: core: Fix serial core controller port name to show controller id")
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Closes: https://lore.kernel.org/oe-lkp/202308021529.35b3ad6c-oliver.sang@intel.com
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> With this patch applied, I see the following traceback in the pending-fixes
> branch.
> 
> Bisect log attached as well. It actually points to commit d962de6ae51f
> ("serial: core: Fix serial core port id to not use port->line").
> Bisect was on mips, but I also see problems on arm, ppc, and sparc.
> sparc boot tests show the warning message and then stall until aborted
> (which of course may be a different problem).

Sorry about all the hassles and thanks for testing again.

I too noticed several issues remaining after testing reloading the hardware
specific serial driver, the issues I saw should be fixed in tty-linus.

> kobject: kobject_add_internal failed for serial8250.0:0.0 with -EEXIST, don't try to register things with the same name in the same directory.
> serial8250 serial8250.0: unable to register port at index 1 (IO2f8 MEM0 IRQ3): -17
> sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'

The issue above should be fixed with commit:

bbb4abb1bcfb ("serial: 8250: Reinit port_id when adding back serial8250_isa_devs")

Not sure about the sparc one you mentioned, but let's when you run your
tests again.

Regards,

Tony
Guenter Roeck Aug. 5, 2023, 10:50 a.m. UTC | #7 
On 8/4/23 21:49, Tony Lindgren wrote:
> * Guenter Roeck <linux@roeck-us.net> [230804 21:42]:
>> On Thu, Aug 03, 2023 at 10:10:32AM +0300, Tony Lindgren wrote:
>>> While fixing DEVNAME to be more usable, I broke serial_base_match() as the
>>> ctrl and port prefix for device names seemed unnecessary.
>>>
>>> The prefixes are still needed by serial_base_match() to probe the serial
>>> base controller port, and serial tx is now broken.
>>>
>>> Let's fix the issue by checking against dev->type and drv->name instead
>>> of the prefixes that are no longer in the DEVNAME.
>>>
>>> Fixes: 1ef2c2df1199 ("serial: core: Fix serial core controller port name to show controller id")
>>> Reported-by: kernel test robot <oliver.sang@intel.com>
>>> Closes: https://lore.kernel.org/oe-lkp/202308021529.35b3ad6c-oliver.sang@intel.com
>>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>>
>> With this patch applied, I see the following traceback in the pending-fixes
>> branch.
>>
>> Bisect log attached as well. It actually points to commit d962de6ae51f
>> ("serial: core: Fix serial core port id to not use port->line").
>> Bisect was on mips, but I also see problems on arm, ppc, and sparc.
>> sparc boot tests show the warning message and then stall until aborted
>> (which of course may be a different problem).
> 
> Sorry about all the hassles and thanks for testing again.
> 
> I too noticed several issues remaining after testing reloading the hardware
> specific serial driver, the issues I saw should be fixed in tty-linus.
> 
>> kobject: kobject_add_internal failed for serial8250.0:0.0 with -EEXIST, don't try to register things with the same name in the same directory.
>> serial8250 serial8250.0: unable to register port at index 1 (IO2f8 MEM0 IRQ3): -17
>> sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'
> 
> The issue above should be fixed with commit:
> 
> bbb4abb1bcfb ("serial: 8250: Reinit port_id when adding back serial8250_isa_devs")
> 

No. I tested the tip of tty-linus (v6.5-rc4-9-gdfe2aeb226fd) which includes
above commit, but still see the problem. sparc images also still stall after
the "cannot create duplicate filename" message.
I bisected the sparc problem - it also bisects to commit d962de6ae51f.

The problem affects all mips boot tests, all sparc boot tests, as well as
arm sx1, ppc bamboo and sam460ex emulations. As far as I can see, those
call serial8250_register_8250_port() without calling  serial8250_setup_port()
and thus don't set port_id. I am only testing a few of those, so I strongly
suspect that all similar callers of serial8250_register_8250_port() are
affected (i.e., almost all of them) if they register more than one serial port.

Guenter
Tony Lindgren Aug. 5, 2023, 11:48 a.m. UTC | #8 
* Guenter Roeck <linux@roeck-us.net> [230805 10:50]:
> On 8/4/23 21:49, Tony Lindgren wrote:
> > * Guenter Roeck <linux@roeck-us.net> [230804 21:42]:
> > > kobject: kobject_add_internal failed for serial8250.0:0.0 with -EEXIST, don't try to register things with the same name in the same directory.
> > > serial8250 serial8250.0: unable to register port at index 1 (IO2f8 MEM0 IRQ3): -17
> > > sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'
> > 
> > The issue above should be fixed with commit:
> > 
> > bbb4abb1bcfb ("serial: 8250: Reinit port_id when adding back serial8250_isa_devs")
> > 
> 
> No. I tested the tip of tty-linus (v6.5-rc4-9-gdfe2aeb226fd) which includes
> above commit, but still see the problem. sparc images also still stall after
> the "cannot create duplicate filename" message.
> I bisected the sparc problem - it also bisects to commit d962de6ae51f.
> 
> The problem affects all mips boot tests, all sparc boot tests, as well as
> arm sx1, ppc bamboo and sam460ex emulations. As far as I can see, those
> call serial8250_register_8250_port() without calling  serial8250_setup_port()
> and thus don't set port_id. I am only testing a few of those, so I strongly
> suspect that all similar callers of serial8250_register_8250_port() are
> affected (i.e., almost all of them) if they register more than one serial port.

OK thanks for explaining. So we need to initialize port->port_id for the
multi-port instances to avoid being stuck with the port->line index. I'll
take a look.

I wonder if we should just revert d962de6ae51f for now. It needs to be
tested to see if something else also needs reverting though.

Regards,

Tony
Guenter Roeck Aug. 5, 2023, 3:50 p.m. UTC | #9 
On 8/5/23 04:48, Tony Lindgren wrote:
> * Guenter Roeck <linux@roeck-us.net> [230805 10:50]:
>> On 8/4/23 21:49, Tony Lindgren wrote:
>>> * Guenter Roeck <linux@roeck-us.net> [230804 21:42]:
>>>> kobject: kobject_add_internal failed for serial8250.0:0.0 with -EEXIST, don't try to register things with the same name in the same directory.
>>>> serial8250 serial8250.0: unable to register port at index 1 (IO2f8 MEM0 IRQ3): -17
>>>> sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'
>>>
>>> The issue above should be fixed with commit:
>>>
>>> bbb4abb1bcfb ("serial: 8250: Reinit port_id when adding back serial8250_isa_devs")
>>>
>>
>> No. I tested the tip of tty-linus (v6.5-rc4-9-gdfe2aeb226fd) which includes
>> above commit, but still see the problem. sparc images also still stall after
>> the "cannot create duplicate filename" message.
>> I bisected the sparc problem - it also bisects to commit d962de6ae51f.
>>
>> The problem affects all mips boot tests, all sparc boot tests, as well as
>> arm sx1, ppc bamboo and sam460ex emulations. As far as I can see, those
>> call serial8250_register_8250_port() without calling  serial8250_setup_port()
>> and thus don't set port_id. I am only testing a few of those, so I strongly
>> suspect that all similar callers of serial8250_register_8250_port() are
>> affected (i.e., almost all of them) if they register more than one serial port.
> 
> OK thanks for explaining. So we need to initialize port->port_id for the
> multi-port instances to avoid being stuck with the port->line index. I'll
> take a look.
> 
> I wonder if we should just revert d962de6ae51f for now. It needs to be
> tested to see if something else also needs reverting though.
> 

It is actually more complicated like that. Adding some debug into, I get the following
from a mips boot. Turns out that serial8250_setup_port() is actually called.

...
####### serial8250_setup_port: index 0
####### serial8250_setup_port: index 0 returning 819a5ab8
####### serial8250_setup_port: index 1
####### serial8250_setup_port: index 1 returning 819a5d20
####### serial8250_setup_port: index 2
####### serial8250_setup_port: index 2 returning 819a5f88
####### serial8250_setup_port: index 3
####### serial8250_setup_port: index 3 returning 819a61f0
...
#### serial8250_register_8250_port: uart=819a5ab8
#### serial8250_register_8250_port: uart=819a5ab8 port_id=0 line=0
...
#### serial8250_register_8250_port: uart=819a5d20
#### serial8250_register_8250_port: uart=819a5d20 port_id=1 line=1
sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'

So line and port_id are both set, but the created sysfs attribute
is still duplicate. I'll do some more debugging.

Guenter
Guenter Roeck Aug. 5, 2023, 4:12 p.m. UTC | #10 
On 8/5/23 08:50, Guenter Roeck wrote:
> On 8/5/23 04:48, Tony Lindgren wrote:
>> * Guenter Roeck <linux@roeck-us.net> [230805 10:50]:
>>> On 8/4/23 21:49, Tony Lindgren wrote:
>>>> * Guenter Roeck <linux@roeck-us.net> [230804 21:42]:
>>>>> kobject: kobject_add_internal failed for serial8250.0:0.0 with -EEXIST, don't try to register things with the same name in the same directory.
>>>>> serial8250 serial8250.0: unable to register port at index 1 (IO2f8 MEM0 IRQ3): -17
>>>>> sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'
>>>>
>>>> The issue above should be fixed with commit:
>>>>
>>>> bbb4abb1bcfb ("serial: 8250: Reinit port_id when adding back serial8250_isa_devs")
>>>>
>>>
>>> No. I tested the tip of tty-linus (v6.5-rc4-9-gdfe2aeb226fd) which includes
>>> above commit, but still see the problem. sparc images also still stall after
>>> the "cannot create duplicate filename" message.
>>> I bisected the sparc problem - it also bisects to commit d962de6ae51f.
>>>
>>> The problem affects all mips boot tests, all sparc boot tests, as well as
>>> arm sx1, ppc bamboo and sam460ex emulations. As far as I can see, those
>>> call serial8250_register_8250_port() without calling  serial8250_setup_port()
>>> and thus don't set port_id. I am only testing a few of those, so I strongly
>>> suspect that all similar callers of serial8250_register_8250_port() are
>>> affected (i.e., almost all of them) if they register more than one serial port.
>>
>> OK thanks for explaining. So we need to initialize port->port_id for the
>> multi-port instances to avoid being stuck with the port->line index. I'll
>> take a look.
>>
>> I wonder if we should just revert d962de6ae51f for now. It needs to be
>> tested to see if something else also needs reverting though.
>>
> 
> It is actually more complicated like that. Adding some debug into, I get the following
> from a mips boot. Turns out that serial8250_setup_port() is actually called.
> 
> ...
> ####### serial8250_setup_port: index 0
> ####### serial8250_setup_port: index 0 returning 819a5ab8
> ####### serial8250_setup_port: index 1
> ####### serial8250_setup_port: index 1 returning 819a5d20
> ####### serial8250_setup_port: index 2
> ####### serial8250_setup_port: index 2 returning 819a5f88
> ####### serial8250_setup_port: index 3
> ####### serial8250_setup_port: index 3 returning 819a61f0
> ...
> #### serial8250_register_8250_port: uart=819a5ab8
> #### serial8250_register_8250_port: uart=819a5ab8 port_id=0 line=0
> ...
> #### serial8250_register_8250_port: uart=819a5d20
> #### serial8250_register_8250_port: uart=819a5d20 port_id=1 line=1
> sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'
> 
> So line and port_id are both set, but the created sysfs attribute
> is still duplicate. I'll do some more debugging.
> 

Ok, it is actually quite simple. In serial8250_register_8250_port(),
uart->port.port_id has the correct and expected value. However, that is
overwritten with
	uart->port.port_id      = up->port.port_id;
where 'up' is the port pointer passed by the caller of serial8250_register_8250_port().
And 'port_id' is always 0 in _that_ port pointer (while 'line' is set correctly).

Guenter
Tony Lindgren Aug. 6, 2023, 4:33 a.m. UTC | #11 
* Guenter Roeck <linux@roeck-us.net> [230805 16:12]:
> On 8/5/23 08:50, Guenter Roeck wrote:
> > On 8/5/23 04:48, Tony Lindgren wrote:
> > > * Guenter Roeck <linux@roeck-us.net> [230805 10:50]:
> > > > On 8/4/23 21:49, Tony Lindgren wrote:
> > > > > * Guenter Roeck <linux@roeck-us.net> [230804 21:42]:
> > > > > > kobject: kobject_add_internal failed for serial8250.0:0.0 with -EEXIST, don't try to register things with the same name in the same directory.
> > > > > > serial8250 serial8250.0: unable to register port at index 1 (IO2f8 MEM0 IRQ3): -17
> > > > > > sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'
> > > > > 
> > > > > The issue above should be fixed with commit:
> > > > > 
> > > > > bbb4abb1bcfb ("serial: 8250: Reinit port_id when adding back serial8250_isa_devs")
> > > > > 
> > > > 
> > > > No. I tested the tip of tty-linus (v6.5-rc4-9-gdfe2aeb226fd) which includes
> > > > above commit, but still see the problem. sparc images also still stall after
> > > > the "cannot create duplicate filename" message.
> > > > I bisected the sparc problem - it also bisects to commit d962de6ae51f.
> > > > 
> > > > The problem affects all mips boot tests, all sparc boot tests, as well as
> > > > arm sx1, ppc bamboo and sam460ex emulations. As far as I can see, those
> > > > call serial8250_register_8250_port() without calling  serial8250_setup_port()
> > > > and thus don't set port_id. I am only testing a few of those, so I strongly
> > > > suspect that all similar callers of serial8250_register_8250_port() are
> > > > affected (i.e., almost all of them) if they register more than one serial port.
> > > 
> > > OK thanks for explaining. So we need to initialize port->port_id for the
> > > multi-port instances to avoid being stuck with the port->line index. I'll
> > > take a look.
> > > 
> > > I wonder if we should just revert d962de6ae51f for now. It needs to be
> > > tested to see if something else also needs reverting though.
> > > 
> > 
> > It is actually more complicated like that. Adding some debug into, I get the following
> > from a mips boot. Turns out that serial8250_setup_port() is actually called.
> > 
> > ...
> > ####### serial8250_setup_port: index 0
> > ####### serial8250_setup_port: index 0 returning 819a5ab8
> > ####### serial8250_setup_port: index 1
> > ####### serial8250_setup_port: index 1 returning 819a5d20
> > ####### serial8250_setup_port: index 2
> > ####### serial8250_setup_port: index 2 returning 819a5f88
> > ####### serial8250_setup_port: index 3
> > ####### serial8250_setup_port: index 3 returning 819a61f0
> > ...
> > #### serial8250_register_8250_port: uart=819a5ab8
> > #### serial8250_register_8250_port: uart=819a5ab8 port_id=0 line=0
> > ...
> > #### serial8250_register_8250_port: uart=819a5d20
> > #### serial8250_register_8250_port: uart=819a5d20 port_id=1 line=1
> > sysfs: cannot create duplicate filename '/devices/platform/serial8250.0/serial8250.0:0/serial8250.0:0.0'
> > 
> > So line and port_id are both set, but the created sysfs attribute
> > is still duplicate. I'll do some more debugging.
> > 
> 
> Ok, it is actually quite simple. In serial8250_register_8250_port(),
> uart->port.port_id has the correct and expected value. However, that is
> overwritten with
> 	uart->port.port_id      = up->port.port_id;
> where 'up' is the port pointer passed by the caller of serial8250_register_8250_port().
> And 'port_id' is always 0 in _that_ port pointer (while 'line' is set correctly).

To me it seems we can't use port->port_id until multiport drivers
initialize it, or set port->port_id automatically with ida_alloc().

Meanwhile, we can just change back to using port->line assuming that
fixes the issue for your tests. This means the port names are broken
like we had in -rc1 but that's a cosmetic issue for now.

Below is what I have in mind for a partial revert of commit d962de6ae51f.

Regards,

Tony

8< ------------------
diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
--- a/drivers/tty/serial/serial_base_bus.c
+++ b/drivers/tty/serial/serial_base_bus.c
@@ -151,7 +151,7 @@ struct serial_port_device *serial_base_port_add(struct uart_port *port,
 	err = serial_base_device_init(port, &port_dev->dev,
 				      &ctrl_dev->dev, &serial_port_type,
 				      serial_base_port_release,
-				      port->ctrl_id, port->port_id);
+				      port->ctrl_id, port->line);
 	if (err)
 		goto err_put_device;
Tony Lindgren Aug. 6, 2023, 6:23 a.m. UTC | #12 
* Tony Lindgren <tony@atomide.com> [230806 07:34]:
> To me it seems we can't use port->port_id until multiport drivers
> initialize it, or set port->port_id automatically with ida_alloc().
> 
> Meanwhile, we can just change back to using port->line assuming that
> fixes the issue for your tests. This means the port names are broken
> like we had in -rc1 but that's a cosmetic issue for now.

Sent it with a proper patch description [0].

Regards,

Tony

[0] https://lore.kernel.org/linux-serial/20230806062052.47737-1-tony@atomide.com/T/#u
diff mbox series

Patch

diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
--- a/drivers/tty/serial/serial_base_bus.c
+++ b/drivers/tty/serial/serial_base_bus.c
@@ -29,9 +29,15 @@  static const struct device_type serial_port_type = {
 
 static int serial_base_match(struct device *dev, struct device_driver *drv)
 {
-	int len = strlen(drv->name);
+	if (dev->type == &serial_ctrl_type &&
+	    str_has_prefix(drv->name, serial_ctrl_type.name))
+		return 1;
 
-	return !strncmp(dev_name(dev), drv->name, len);
+	if (dev->type == &serial_port_type &&
+	    str_has_prefix(drv->name, serial_port_type.name))
+		return 1;
+
+	return 0;
 }
 
 static struct bus_type serial_base_bus_type = {