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@xxxxxxxxx>
> > > > ---
> > > > 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
