Re: [PATCH v2 net] ptp: ocp: adjust serial port symlink creation

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, May 23, 2024 at 10:06:05PM +0100, Vadim Fedorenko wrote:
> On 23/05/2024 17:26, Greg Kroah-Hartman wrote:
> > On Thu, May 23, 2024 at 08:39:43AM -0700, Jakub Kicinski wrote:
> > > On Fri, 10 May 2024 11:04:05 +0000 Vadim Fedorenko wrote:
> > > > The commit b286f4e87e32 ("serial: core: Move tty and serdev to be children
> > > > of serial core port device") changed the hierarchy of serial port devices
> > > > and device_find_child_by_name cannot find ttyS* devices because they are
> > > > no longer directly attached. Add some logic to restore symlinks creation
> > > > to the driver for OCP TimeCard.
> > > > 
> > > > Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device")
> > > > Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> > > > ---
> > > > v2:
> > > >   add serial/8250 maintainers
> > > > ---
> > > >   drivers/ptp/ptp_ocp.c | 30 +++++++++++++++++++++---------
> > > >   1 file changed, 21 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> > > > index ee2ced88ab34..50b7cb9db3be 100644
> > > > --- a/drivers/ptp/ptp_ocp.c
> > > > +++ b/drivers/ptp/ptp_ocp.c
> > > > @@ -25,6 +25,8 @@
> > > >   #include <linux/crc16.h>
> > > >   #include <linux/dpll.h>
> > > > +#include "../tty/serial/8250/8250.h"
> > > 
> > > Hi Greg, Jiri, does this look reasonable to you?
> > > The cross tree include raises an obvious red flag.
> > 
> > Yeah, that looks wrong.
> > 
> > > Should serial / u8250 provide a more official API?
> > 
> > If it needs to, but why is this driver poking around in here at all?
> 
> Hi Greg!
> 
> Well, the original idea was to have symlinks with self-explanatory names
> to real serial devices exposed by PCIe device.

Why is that needed?  What is wrong with the normal device topology in
/sys/devices/ that shows this already?

> > > Can we use device_for_each_child() to deal with the extra
> > > layer in the hierarchy?
> > 
> > Or a real function where needed?
> 
> yep.
> 
> > 
> > > 
> > > >   #define PCI_VENDOR_ID_FACEBOOK			0x1d9b
> > > >   #define PCI_DEVICE_ID_FACEBOOK_TIMECARD		0x0400
> > > > @@ -4330,11 +4332,9 @@ ptp_ocp_symlink(struct ptp_ocp *bp, struct device *child, const char *link)
> > > >   }
> > > >   static void
> > > > -ptp_ocp_link_child(struct ptp_ocp *bp, const char *name, const char *link)
> > > > +ptp_ocp_link_child(struct ptp_ocp *bp, struct device *dev, const char *name, const char *link)
> > > >   {
> > > > -	struct device *dev, *child;
> > > > -
> > > > -	dev = &bp->pdev->dev;
> > > > +	struct device *child;
> > > >   	child = device_find_child_by_name(dev, name);
> > > >   	if (!child) {
> > > > @@ -4349,27 +4349,39 @@ ptp_ocp_link_child(struct ptp_ocp *bp, const char *name, const char *link)
> > > >   static int
> > > >   ptp_ocp_complete(struct ptp_ocp *bp)
> > > >   {
> > > > +	struct device *dev, *port_dev;
> > > > +	struct uart_8250_port *port;
> > > >   	struct pps_device *pps;
> > > >   	char buf[32];
> > > > +	dev = &bp->pdev->dev;
> > > > +
> > > >   	if (bp->gnss_port.line != -1) {
> > > > +		port = serial8250_get_port(bp->gnss_port.line);
> > > > +		port_dev = (struct device *)port->port.port_dev;
> > 
> > That cast is not going to go well.  How do you know this is always
> > true?
> 
> AFAIU, port_dev starts with struct dev always. That's why it's safe.
> 
> > 
> > What was the original code attempting to do?  It feels like that was
> > wrong to start with if merely moving things around the device tree
> > caused anything to break here.  That is not how the driver core is
> > supposed to be used at all.
> > 
> 
> We just want to have a symlink with meaningful name to real tty device,
> exposed by PCIe device. We provide up to 4 serial ports - GNSS, GNSS2,
> MAC and NMEA, to user space and we don't want user space to guess which
> one is which. We do have user space application which relies on symlinks
> to discover features.

Just use the normal serial topology please.

And for your named devices, use the symlinks in /dev/serial/ that are
provided for you already.

> We don't use device tree because it's PCIe device with pre-defined
> functions, so I don't see any other way to get this info and properly
> create symlinks.

Sorry, I didn't mean "dt", I mean /sys/devices/, there should not be
anything "special" about this driver that requires custom symlinks in
sysfs.  That's for userspace to create in /dev/serial/ as it does today.
Please just remove all of this, as it's not a good idea at all.

thanks,

greg k-h