Re: [PATCH 00/13] tty.h cleanups
On Thu, Apr 15, 2021 at 04:14:33PM +0200, Johan Hovold wrote: > On Thu, Apr 15, 2021 at 10:21:54AM +0200, Greg Kroah-Hartman wrote: > > On Fri, Apr 09, 2021 at 09:32:45AM +0200, Johan Hovold wrote: > > > On Thu, Apr 08, 2021 at 08:01:08PM +0200, Greg Kroah-Hartman wrote: > > > > On Thu, Apr 08, 2021 at 04:25:22PM +0200, Johan Hovold wrote: > > > > > On Thu, Apr 08, 2021 at 02:51:21PM +0200, Greg Kroah-Hartman wrote: > > > > > > Turns out there is a lot of tty-internal stuff in > > > > > > include/linux/tty.h > > > > > > that do not belong there. Create a internal-to-the-tty-layer .h > > > > > > file > > > > > > for these types of things and move function prototypes to it > > > > > > instead of > > > > > > being in the system-wide header file. > > > > > > > > > > > > Along the way clean up the use of some old tty-only debugging > > > > > > macros and > > > > > > use the in-kernel dev_*() calls instead. > > > > > > > > > > I'm afraid that's not a good idea since not all ttys have a > > > > > corresponding class device. Notable exception include pseudo terminals > > > > > and serdev. > > > > > > > > > > While dev_printk() can handle a NULL device argument without crashing, > > > > > we'll actually lose log information by removing the tty printk > > > > > helpers. > > > > > > > > I think the same info will be printed here as before, just some NULL > > > > information at the beginning, right? And the benifits overall (for real > > > > tty devices), should outweigh the few devices that do not have this > > > > information. > > > > > > No, you'll only be losing information (tty driver and tty name). Here's > > > a pty example, where the first line in each pair use dev_info() and the > > > second tty_info(): > > > > > > [ 10.235331] (NULL device *): tty_get_device > > > [ 10.235441] ptm ptm0: tty_get_device > > > > > > [ 10.235586] (NULL device *): tty_get_device > > > [ 10.235674] pts pts0: tty_get_device > > > > > > and similar for serdev, which is becoming more and more common. > > > > Ok, good point, I'll go apply only the first 2 patches in this series > > (moving the macros out of tty.h and removing the unused one) and then > > will redo this set of patches again. > > Perhaps no harm in leaving the tty_info() on in there for consistency. > We have users of the _ratelimited() flavour of it (even if there's no > dependency). I dropped it, no need to keep around unused macros :) > > I think a better tty_msg() macro is warrented so that we can provide > > dev_*() output if we have a device, otherwise fall back to the old > > style to preserve functionality. > > Possibly, but the dev_printk() for the tty class devices wouldn't > provide any more info than what's already there (i.e. driver name + tty > name). > > (And associating ttys with other devices and drivers (e.g. a serdev > client and its driver) might not be what we want since you lose the > connection to the underlying tty driver.) Yeah, I messed with this for a bit today and I think I'm just going to give up and leave it as-is for now... thanks, greg k-h
Re: [PATCH 00/13] tty.h cleanups
On Thu, Apr 15, 2021 at 10:21:54AM +0200, Greg Kroah-Hartman wrote: > On Fri, Apr 09, 2021 at 09:32:45AM +0200, Johan Hovold wrote: > > On Thu, Apr 08, 2021 at 08:01:08PM +0200, Greg Kroah-Hartman wrote: > > > On Thu, Apr 08, 2021 at 04:25:22PM +0200, Johan Hovold wrote: > > > > On Thu, Apr 08, 2021 at 02:51:21PM +0200, Greg Kroah-Hartman wrote: > > > > > Turns out there is a lot of tty-internal stuff in include/linux/tty.h > > > > > that do not belong there. Create a internal-to-the-tty-layer .h file > > > > > for these types of things and move function prototypes to it instead > > > > > of > > > > > being in the system-wide header file. > > > > > > > > > > Along the way clean up the use of some old tty-only debugging macros > > > > > and > > > > > use the in-kernel dev_*() calls instead. > > > > > > > > I'm afraid that's not a good idea since not all ttys have a > > > > corresponding class device. Notable exception include pseudo terminals > > > > and serdev. > > > > > > > > While dev_printk() can handle a NULL device argument without crashing, > > > > we'll actually lose log information by removing the tty printk helpers. > > > > > > I think the same info will be printed here as before, just some NULL > > > information at the beginning, right? And the benifits overall (for real > > > tty devices), should outweigh the few devices that do not have this > > > information. > > > > No, you'll only be losing information (tty driver and tty name). Here's > > a pty example, where the first line in each pair use dev_info() and the > > second tty_info(): > > > > [ 10.235331] (NULL device *): tty_get_device > > [ 10.235441] ptm ptm0: tty_get_device > > > > [ 10.235586] (NULL device *): tty_get_device > > [ 10.235674] pts pts0: tty_get_device > > > > and similar for serdev, which is becoming more and more common. > > Ok, good point, I'll go apply only the first 2 patches in this series > (moving the macros out of tty.h and removing the unused one) and then > will redo this set of patches again. Perhaps no harm in leaving the tty_info() on in there for consistency. We have users of the _ratelimited() flavour of it (even if there's no dependency). > I think a better tty_msg() macro is warrented so that we can provide > dev_*() output if we have a device, otherwise fall back to the old > style to preserve functionality. Possibly, but the dev_printk() for the tty class devices wouldn't provide any more info than what's already there (i.e. driver name + tty name). (And associating ttys with other devices and drivers (e.g. a serdev client and its driver) might not be what we want since you lose the connection to the underlying tty driver.) Johan
Re: [PATCH 00/13] tty.h cleanups
On Fri, Apr 09, 2021 at 09:32:45AM +0200, Johan Hovold wrote: > On Thu, Apr 08, 2021 at 08:01:08PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Apr 08, 2021 at 04:25:22PM +0200, Johan Hovold wrote: > > > On Thu, Apr 08, 2021 at 02:51:21PM +0200, Greg Kroah-Hartman wrote: > > > > Turns out there is a lot of tty-internal stuff in include/linux/tty.h > > > > that do not belong there. Create a internal-to-the-tty-layer .h file > > > > for these types of things and move function prototypes to it instead of > > > > being in the system-wide header file. > > > > > > > > Along the way clean up the use of some old tty-only debugging macros and > > > > use the in-kernel dev_*() calls instead. > > > > > > I'm afraid that's not a good idea since not all ttys have a > > > corresponding class device. Notable exception include pseudo terminals > > > and serdev. > > > > > > While dev_printk() can handle a NULL device argument without crashing, > > > we'll actually lose log information by removing the tty printk helpers. > > > > I think the same info will be printed here as before, just some NULL > > information at the beginning, right? And the benifits overall (for real > > tty devices), should outweigh the few devices that do not have this > > information. > > No, you'll only be losing information (tty driver and tty name). Here's > a pty example, where the first line in each pair use dev_info() and the > second tty_info(): > > [ 10.235331] (NULL device *): tty_get_device > [ 10.235441] ptm ptm0: tty_get_device > > [ 10.235586] (NULL device *): tty_get_device > [ 10.235674] pts pts0: tty_get_device > > and similar for serdev, which is becoming more and more common. Ok, good point, I'll go apply only the first 2 patches in this series (moving the macros out of tty.h and removing the unused one) and then will redo this set of patches again. I think a better tty_msg() macro is warrented so that we can provide dev_*() output if we have a device, otherwise fall back to the old style to preserve functionality. thanks, greg k-h
Re: [PATCH 00/13] tty.h cleanups
On Thu, Apr 08, 2021 at 08:01:08PM +0200, Greg Kroah-Hartman wrote: > On Thu, Apr 08, 2021 at 04:25:22PM +0200, Johan Hovold wrote: > > On Thu, Apr 08, 2021 at 02:51:21PM +0200, Greg Kroah-Hartman wrote: > > > Turns out there is a lot of tty-internal stuff in include/linux/tty.h > > > that do not belong there. Create a internal-to-the-tty-layer .h file > > > for these types of things and move function prototypes to it instead of > > > being in the system-wide header file. > > > > > > Along the way clean up the use of some old tty-only debugging macros and > > > use the in-kernel dev_*() calls instead. > > > > I'm afraid that's not a good idea since not all ttys have a > > corresponding class device. Notable exception include pseudo terminals > > and serdev. > > > > While dev_printk() can handle a NULL device argument without crashing, > > we'll actually lose log information by removing the tty printk helpers. > > I think the same info will be printed here as before, just some NULL > information at the beginning, right? And the benifits overall (for real > tty devices), should outweigh the few devices that do not have this > information. No, you'll only be losing information (tty driver and tty name). Here's a pty example, where the first line in each pair use dev_info() and the second tty_info(): [ 10.235331] (NULL device *): tty_get_device [ 10.235441] ptm ptm0: tty_get_device [ 10.235586] (NULL device *): tty_get_device [ 10.235674] pts pts0: tty_get_device and similar for serdev, which is becoming more and more common. Johan
Re: [PATCH 00/13] tty.h cleanups
On Thu, Apr 08, 2021 at 04:25:22PM +0200, Johan Hovold wrote: > On Thu, Apr 08, 2021 at 02:51:21PM +0200, Greg Kroah-Hartman wrote: > > Turns out there is a lot of tty-internal stuff in include/linux/tty.h > > that do not belong there. Create a internal-to-the-tty-layer .h file > > for these types of things and move function prototypes to it instead of > > being in the system-wide header file. > > > > Along the way clean up the use of some old tty-only debugging macros and > > use the in-kernel dev_*() calls instead. > > I'm afraid that's not a good idea since not all ttys have a > corresponding class device. Notable exception include pseudo terminals > and serdev. > > While dev_printk() can handle a NULL device argument without crashing, > we'll actually lose log information by removing the tty printk helpers. I think the same info will be printed here as before, just some NULL information at the beginning, right? And the benifits overall (for real tty devices), should outweigh the few devices that do not have this information. But let me run some tests, on those devices to see just how this looks... thanks, greg k-h
Re: [PATCH 00/13] tty.h cleanups
On Thu, Apr 08, 2021 at 02:51:21PM +0200, Greg Kroah-Hartman wrote: > Turns out there is a lot of tty-internal stuff in include/linux/tty.h > that do not belong there. Create a internal-to-the-tty-layer .h file > for these types of things and move function prototypes to it instead of > being in the system-wide header file. > > Along the way clean up the use of some old tty-only debugging macros and > use the in-kernel dev_*() calls instead. I'm afraid that's not a good idea since not all ttys have a corresponding class device. Notable exception include pseudo terminals and serdev. While dev_printk() can handle a NULL device argument without crashing, we'll actually lose log information by removing the tty printk helpers. Johan
