[PATCH net v2 1/2] ptp: ocp: adjust sysfs entries to expose tty information

[PATCH net v2 1/2] ptp: ocp: adjust sysfs entries to expose tty information

[Vadim Fedorenko]

Starting v6.8 the serial port subsystem changed the hierarchy of devices
and symlinks are not working anymore. Previous discussion made it clear
that the idea of symlinks for tty devices was wrong by design. Implement
additional attributes to expose the information. Fixes tag points to the
commit which introduced the change.

Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device")
Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 drivers/ptp/ptp_ocp.c | 68 +++++++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 16 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index ee2ced88ab34..7a5026656452 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -3346,6 +3346,55 @@ static EXT_ATTR_RO(freq, frequency, 1);
 static EXT_ATTR_RO(freq, frequency, 2);
 static EXT_ATTR_RO(freq, frequency, 3);
 
+static ssize_t
+ptp_ocp_tty_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct dev_ext_attribute *ea = to_ext_attr(attr);
+	struct ptp_ocp *bp = dev_get_drvdata(dev);
+	struct ptp_ocp_serial_port *port;
+
+	port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
+	return sysfs_emit(buf, "ttyS%d", port->line);
+}
+
+static umode_t
+ptp_ocp_timecard_tty_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
+	struct ptp_ocp_serial_port *port;
+	struct device_attribute *dattr;
+	struct dev_ext_attribute *ea;
+
+	if (strncmp(attr->name, "tty", 3))
+		return attr->mode;
+
+	dattr = container_of(attr, struct device_attribute, attr);
+	ea = container_of(dattr, struct dev_ext_attribute, attr);
+	port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
+	return port->line == -1 ? 0 : 0444;
+}
+#define EXT_TTY_ATTR_RO(_name, _val)			\
+	struct dev_ext_attribute dev_attr_tty##_name =	\
+		{ __ATTR(tty##_name, 0444, ptp_ocp_tty_show, NULL), (void *)_val }
+
+static EXT_TTY_ATTR_RO(GNSS, offsetof(struct ptp_ocp, gnss_port));
+static EXT_TTY_ATTR_RO(GNSS2, offsetof(struct ptp_ocp, gnss2_port));
+static EXT_TTY_ATTR_RO(MAC, offsetof(struct ptp_ocp, mac_port));
+static EXT_TTY_ATTR_RO(NMEA, offsetof(struct ptp_ocp, nmea_port));
+static struct attribute *ptp_ocp_timecard_tty_attrs[] = {
+	&dev_attr_ttyGNSS.attr.attr,
+	&dev_attr_ttyGNSS2.attr.attr,
+	&dev_attr_ttyMAC.attr.attr,
+	&dev_attr_ttyNMEA.attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ptp_ocp_timecard_tty_group = {
+	.name = "tty",
+	.attrs = ptp_ocp_timecard_tty_attrs,
+	.is_visible = ptp_ocp_timecard_tty_is_visible,
+};
+
 static ssize_t
 serialnum_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -3775,6 +3824,7 @@ static const struct attribute_group fb_timecard_group = {
 
 static const struct ocp_attr_group fb_timecard_groups[] = {
 	{ .cap = OCP_CAP_BASIC,	    .group = &fb_timecard_group },
+	{ .cap = OCP_CAP_BASIC,	    .group = &ptp_ocp_timecard_tty_group },
 	{ .cap = OCP_CAP_SIGNAL,    .group = &fb_timecard_signal0_group },
 	{ .cap = OCP_CAP_SIGNAL,    .group = &fb_timecard_signal1_group },
 	{ .cap = OCP_CAP_SIGNAL,    .group = &fb_timecard_signal2_group },
@@ -3814,6 +3864,7 @@ static const struct attribute_group art_timecard_group = {
 
 static const struct ocp_attr_group art_timecard_groups[] = {
 	{ .cap = OCP_CAP_BASIC,	    .group = &art_timecard_group },
+	{ .cap = OCP_CAP_BASIC,	    .group = &ptp_ocp_timecard_tty_group },
 	{ },
 };
 
@@ -3841,6 +3892,7 @@ static const struct attribute_group adva_timecard_group = {
 
 static const struct ocp_attr_group adva_timecard_groups[] = {
 	{ .cap = OCP_CAP_BASIC,	    .group = &adva_timecard_group },
+	{ .cap = OCP_CAP_BASIC,	    .group = &ptp_ocp_timecard_tty_group },
 	{ .cap = OCP_CAP_SIGNAL,    .group = &fb_timecard_signal0_group },
 	{ .cap = OCP_CAP_SIGNAL,    .group = &fb_timecard_signal1_group },
 	{ .cap = OCP_CAP_FREQ,	    .group = &fb_timecard_freq0_group },
@@ -4352,22 +4404,6 @@ ptp_ocp_complete(struct ptp_ocp *bp)
 	struct pps_device *pps;
 	char buf[32];
 
-	if (bp->gnss_port.line != -1) {
-		sprintf(buf, "ttyS%d", bp->gnss_port.line);
-		ptp_ocp_link_child(bp, buf, "ttyGNSS");
-	}
-	if (bp->gnss2_port.line != -1) {
-		sprintf(buf, "ttyS%d", bp->gnss2_port.line);
-		ptp_ocp_link_child(bp, buf, "ttyGNSS2");
-	}
-	if (bp->mac_port.line != -1) {
-		sprintf(buf, "ttyS%d", bp->mac_port.line);
-		ptp_ocp_link_child(bp, buf, "ttyMAC");
-	}
-	if (bp->nmea_port.line != -1) {
-		sprintf(buf, "ttyS%d", bp->nmea_port.line);
-		ptp_ocp_link_child(bp, buf, "ttyNMEA");
-	}
 	sprintf(buf, "ptp%d", ptp_clock_index(bp->ptp));
 	ptp_ocp_link_child(bp, buf, "ptp");
 
-- 
2.43.5

[PATCH net v2 2/2] docs: ABI: update OCP TimeCard sysfs entries

[Vadim Fedorenko]

Update documentation according to the changes in the driver.

New attributes group tty is exposed and ttyGNSS, ttyGNSS2, ttyMAC and
ttyNMEA are moved to this gruop. Also, these attributes are no more
links to the devices but rather simple text files containing names of
tty devices.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 Documentation/ABI/testing/sysfs-timecard | 31 ++++++++++++++----------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
index 220478156297..3ae41b7634ac 100644
--- a/Documentation/ABI/testing/sysfs-timecard
+++ b/Documentation/ABI/testing/sysfs-timecard
@@ -258,24 +258,29 @@ Description:	(RW) When retrieving the PHC with the PTP SYS_OFFSET_EXTENDED
 		the estimated point where the FPGA latches the PHC time.  This
 		value may be changed by writing an unsigned integer.
 
-What:		/sys/class/timecard/ocpN/ttyGNSS
-What:		/sys/class/timecard/ocpN/ttyGNSS2
-Date:		September 2021
+What:		/sys/class/timecard/ocpN/tty
+Date:		August 2024
+Contact:	Vadim Fedorenko <vadim.fedorenko@linux.dev>
+Description:	(RO) Directory containing the sysfs nodes for TTY attributes
+
+What:		/sys/class/timecard/ocpN/tty/ttyGNSS
+What:		/sys/class/timecard/ocpN/tty/ttyGNSS2
+Date:		August 2024
 Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
-Description:	These optional attributes link to the TTY serial ports
-		associated with the GNSS devices.
+Description:	(RO) These optional attributes contain names of the TTY serial
+		ports associated with the GNSS devices.
 
-What:		/sys/class/timecard/ocpN/ttyMAC
-Date:		September 2021
+What:		/sys/class/timecard/ocpN/tty/ttyMAC
+Date:		August 2024
 Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
-Description:	This optional attribute links to the TTY serial port
-		associated with the Miniature Atomic Clock.
+Description:	(RO) This optional attribute contains name of the TTY serial
+		port associated with the Miniature Atomic Clock.
 
-What:		/sys/class/timecard/ocpN/ttyNMEA
-Date:		September 2021
+What:		/sys/class/timecard/ocpN/tty/ttyNMEA
+Date:		August 2024
 Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
-Description:	This optional attribute links to the TTY serial port
-		which outputs the PHC time in NMEA ZDA format.
+Description:	(RO) This optional attribute contains name of the TTY serial
+		port which outputs the PHC time in NMEA ZDA format.
 
 What:		/sys/class/timecard/ocpN/utc_tai_offset
 Date:		September 2021
-- 
2.43.5

Re: [PATCH net v2 2/2] docs: ABI: update OCP TimeCard sysfs entries

[Simon Horman]

On Mon, Aug 05, 2024 at 03:05:00PM -0700, Vadim Fedorenko wrote:
> Update documentation according to the changes in the driver.
> 
> New attributes group tty is exposed and ttyGNSS, ttyGNSS2, ttyMAC and
> ttyNMEA are moved to this gruop. Also, these attributes are no more

nit: group

     Flagged by checkpatch --codespell

> links to the devices but rather simple text files containing names of
> tty devices.
> 
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>

...

Re: [PATCH net v2 1/2] ptp: ocp: adjust sysfs entries to expose tty information

[Greg Kroah-Hartman]

On Mon, Aug 05, 2024 at 03:04:59PM -0700, Vadim Fedorenko wrote:
> Starting v6.8 the serial port subsystem changed the hierarchy of devices
> and symlinks are not working anymore. Previous discussion made it clear
> that the idea of symlinks for tty devices was wrong by design. Implement
> additional attributes to expose the information. Fixes tag points to the
> commit which introduced the change.
> 
> Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device")
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
>  drivers/ptp/ptp_ocp.c | 68 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 52 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index ee2ced88ab34..7a5026656452 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -3346,6 +3346,55 @@ static EXT_ATTR_RO(freq, frequency, 1);
>  static EXT_ATTR_RO(freq, frequency, 2);
>  static EXT_ATTR_RO(freq, frequency, 3);
>  
> +static ssize_t
> +ptp_ocp_tty_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct dev_ext_attribute *ea = to_ext_attr(attr);
> +	struct ptp_ocp *bp = dev_get_drvdata(dev);
> +	struct ptp_ocp_serial_port *port;
> +
> +	port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);

That's insane pointer math, how do we know this is correct?

> +	return sysfs_emit(buf, "ttyS%d", port->line);
> +}
> +
> +static umode_t
> +ptp_ocp_timecard_tty_is_visible(struct kobject *kobj, struct attribute *attr, int n)
> +{
> +	struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
> +	struct ptp_ocp_serial_port *port;
> +	struct device_attribute *dattr;
> +	struct dev_ext_attribute *ea;
> +
> +	if (strncmp(attr->name, "tty", 3))
> +		return attr->mode;
> +
> +	dattr = container_of(attr, struct device_attribute, attr);
> +	ea = container_of(dattr, struct dev_ext_attribute, attr);
> +	port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);

That's crazy pointer math, how are you ensured that it is correct?  Why
isn't there a container_of() thing here instead?

thanks,

greg k-h

Re: [PATCH net v2 1/2] ptp: ocp: adjust sysfs entries to expose tty information

[Vadim Fedorenko]

On 13/08/2024 10:33, Greg Kroah-Hartman wrote:
> On Mon, Aug 05, 2024 at 03:04:59PM -0700, Vadim Fedorenko wrote:
>> Starting v6.8 the serial port subsystem changed the hierarchy of devices
>> and symlinks are not working anymore. Previous discussion made it clear
>> that the idea of symlinks for tty devices was wrong by design. Implement
>> additional attributes to expose the information. Fixes tag points to the
>> commit which introduced the change.
>>
>> Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device")
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>> ---
>>   drivers/ptp/ptp_ocp.c | 68 +++++++++++++++++++++++++++++++++----------
>>   1 file changed, 52 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>> index ee2ced88ab34..7a5026656452 100644
>> --- a/drivers/ptp/ptp_ocp.c
>> +++ b/drivers/ptp/ptp_ocp.c
>> @@ -3346,6 +3346,55 @@ static EXT_ATTR_RO(freq, frequency, 1);
>>   static EXT_ATTR_RO(freq, frequency, 2);
>>   static EXT_ATTR_RO(freq, frequency, 3);
>>   
>> +static ssize_t
>> +ptp_ocp_tty_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	struct dev_ext_attribute *ea = to_ext_attr(attr);
>> +	struct ptp_ocp *bp = dev_get_drvdata(dev);
>> +	struct ptp_ocp_serial_port *port;
>> +
>> +	port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
> 
> That's insane pointer math, how do we know this is correct?
> 
>> +	return sysfs_emit(buf, "ttyS%d", port->line);
>> +}
>> +
>> +static umode_t
>> +ptp_ocp_timecard_tty_is_visible(struct kobject *kobj, struct attribute *attr, int n)
>> +{
>> +	struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
>> +	struct ptp_ocp_serial_port *port;
>> +	struct device_attribute *dattr;
>> +	struct dev_ext_attribute *ea;
>> +
>> +	if (strncmp(attr->name, "tty", 3))
>> +		return attr->mode;
>> +
>> +	dattr = container_of(attr, struct device_attribute, attr);
>> +	ea = container_of(dattr, struct dev_ext_attribute, attr);
>> +	port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
> 
> That's crazy pointer math, how are you ensured that it is correct?  Why
> isn't there a container_of() thing here instead?

Well, container_of cannot be used here because the attributes are static
while the function reads dynamic instance. The only values that are
populated into the attributes of the group are offsets.
But I can convert it to a helper which will check that the offset 
provided is the real offset of the structure we expect. And it could be 
reused in both "is_visible" and "show" functions.

> thanks,
> 
> greg k-h

Re: [PATCH net v2 1/2] ptp: ocp: adjust sysfs entries to expose tty information

[Jiri Slaby]

On 13. 08. 24, 20:24, Vadim Fedorenko wrote:
> On 13/08/2024 10:33, Greg Kroah-Hartman wrote:
>> On Mon, Aug 05, 2024 at 03:04:59PM -0700, Vadim Fedorenko wrote:
>>> Starting v6.8 the serial port subsystem changed the hierarchy of devices
>>> and symlinks are not working anymore. Previous discussion made it clear
>>> that the idea of symlinks for tty devices was wrong by design. Implement
>>> additional attributes to expose the information. Fixes tag points to the
>>> commit which introduced the change.
>>>
>>> Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be 
>>> children of serial core port device")
>>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>>> ---
>>>   drivers/ptp/ptp_ocp.c | 68 +++++++++++++++++++++++++++++++++----------
>>>   1 file changed, 52 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>> index ee2ced88ab34..7a5026656452 100644
>>> --- a/drivers/ptp/ptp_ocp.c
>>> +++ b/drivers/ptp/ptp_ocp.c
>>> @@ -3346,6 +3346,55 @@ static EXT_ATTR_RO(freq, frequency, 1);
>>>   static EXT_ATTR_RO(freq, frequency, 2);
>>>   static EXT_ATTR_RO(freq, frequency, 3);
>>> +static ssize_t
>>> +ptp_ocp_tty_show(struct device *dev, struct device_attribute *attr, 
>>> char *buf)
>>> +{
>>> +    struct dev_ext_attribute *ea = to_ext_attr(attr);
>>> +    struct ptp_ocp *bp = dev_get_drvdata(dev);
>>> +    struct ptp_ocp_serial_port *port;
>>> +
>>> +    port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
>>
>> That's insane pointer math, how do we know this is correct?
>>
>>> +    return sysfs_emit(buf, "ttyS%d", port->line);
>>> +}
>>> +
>>> +static umode_t
>>> +ptp_ocp_timecard_tty_is_visible(struct kobject *kobj, struct 
>>> attribute *attr, int n)
>>> +{
>>> +    struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
>>> +    struct ptp_ocp_serial_port *port;
>>> +    struct device_attribute *dattr;
>>> +    struct dev_ext_attribute *ea;
>>> +
>>> +    if (strncmp(attr->name, "tty", 3))
>>> +        return attr->mode;
>>> +
>>> +    dattr = container_of(attr, struct device_attribute, attr);
>>> +    ea = container_of(dattr, struct dev_ext_attribute, attr);
>>> +    port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
>>
>> That's crazy pointer math, how are you ensured that it is correct?  Why
>> isn't there a container_of() thing here instead?
> 
> Well, container_of cannot be used here because the attributes are static
> while the function reads dynamic instance. The only values that are
> populated into the attributes of the group are offsets.
> But I can convert it to a helper which will check that the offset 
> provided is the real offset of the structure we expect. And it could be 
> reused in both "is_visible" and "show" functions.

Strong NACK against this approach.

What about converting those 4 ports into an array and adding an enum { 
PORT_GNSS, POTR_GNSS2, PORT_MAC, PORT_NMEA }?

thanks,
-- 
js
suse labs

Re: [PATCH net v2 1/2] ptp: ocp: adjust sysfs entries to expose tty information

[Vadim Fedorenko]

On 14/08/2024 06:00, Jiri Slaby wrote:
> On 13. 08. 24, 20:24, Vadim Fedorenko wrote:
>> On 13/08/2024 10:33, Greg Kroah-Hartman wrote:
>>> On Mon, Aug 05, 2024 at 03:04:59PM -0700, Vadim Fedorenko wrote:
>>>> Starting v6.8 the serial port subsystem changed the hierarchy of 
>>>> devices
>>>> and symlinks are not working anymore. Previous discussion made it clear
>>>> that the idea of symlinks for tty devices was wrong by design. 
>>>> Implement
>>>> additional attributes to expose the information. Fixes tag points to 
>>>> the
>>>> commit which introduced the change.
>>>>
>>>> Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be 
>>>> children of serial core port device")
>>>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>>>> ---
>>>>   drivers/ptp/ptp_ocp.c | 68 
>>>> +++++++++++++++++++++++++++++++++----------
>>>>   1 file changed, 52 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>>> index ee2ced88ab34..7a5026656452 100644
>>>> --- a/drivers/ptp/ptp_ocp.c
>>>> +++ b/drivers/ptp/ptp_ocp.c
>>>> @@ -3346,6 +3346,55 @@ static EXT_ATTR_RO(freq, frequency, 1);
>>>>   static EXT_ATTR_RO(freq, frequency, 2);
>>>>   static EXT_ATTR_RO(freq, frequency, 3);
>>>> +static ssize_t
>>>> +ptp_ocp_tty_show(struct device *dev, struct device_attribute *attr, 
>>>> char *buf)
>>>> +{
>>>> +    struct dev_ext_attribute *ea = to_ext_attr(attr);
>>>> +    struct ptp_ocp *bp = dev_get_drvdata(dev);
>>>> +    struct ptp_ocp_serial_port *port;
>>>> +
>>>> +    port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
>>>
>>> That's insane pointer math, how do we know this is correct?
>>>
>>>> +    return sysfs_emit(buf, "ttyS%d", port->line);
>>>> +}
>>>> +
>>>> +static umode_t
>>>> +ptp_ocp_timecard_tty_is_visible(struct kobject *kobj, struct 
>>>> attribute *attr, int n)
>>>> +{
>>>> +    struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
>>>> +    struct ptp_ocp_serial_port *port;
>>>> +    struct device_attribute *dattr;
>>>> +    struct dev_ext_attribute *ea;
>>>> +
>>>> +    if (strncmp(attr->name, "tty", 3))
>>>> +        return attr->mode;
>>>> +
>>>> +    dattr = container_of(attr, struct device_attribute, attr);
>>>> +    ea = container_of(dattr, struct dev_ext_attribute, attr);
>>>> +    port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
>>>
>>> That's crazy pointer math, how are you ensured that it is correct?  Why
>>> isn't there a container_of() thing here instead?
>>
>> Well, container_of cannot be used here because the attributes are static
>> while the function reads dynamic instance. The only values that are
>> populated into the attributes of the group are offsets.
>> But I can convert it to a helper which will check that the offset 
>> provided is the real offset of the structure we expect. And it could 
>> be reused in both "is_visible" and "show" functions.
> 
> Strong NACK against this approach.
> 
> What about converting those 4 ports into an array and adding an enum { 
> PORT_GNSS, POTR_GNSS2, PORT_MAC, PORT_NMEA }?

Why is it a problem? I don't see big difference between these 2
implementations:

struct ptp_ocp_serial_port *get_port(struct ptp_ocp *bp, void *offset)
{
	switch((uintptr_t)offset) {
		case offsetof(struct ptp_ocp, gnss_port):
			return &bp->gnss_port;
		case offsetof(struct ptp_ocp, gnss2_port):
			return &bp->gnss2_port;
		case offsetof(struct ptp_ocp, mac_port):
			return &bp->mac_port;
		case offsetof(struct ptp_ocp, nmea_port):
			return &bp->nmea_port;
	}
	return NULL;
}

and:

struct ptp_ocp_serial_port *get_port(struct ptp_ocp *bp, void *offset)
{
	switch((enum port_type)offset) {
		case PORT_GNSS:
			return &bp->tty_port[PORT_GNSS];
		case PORT_GNSS2:
			return &bp->tty_port[PORT_GNSS2];
		case PORT_MAC:
			return &bp->tty_port[PORT_MAC];
		case PORT_NMEA:
			return &bp->tty_port[PORT_NMEA];
	}
	return NULL;
}

The second option will require more LoC to change the initialization
part of the driver, but will not simplify the access.
If you suggest to use enum value directly, without the check, then
it will not solve the problem of checking the boundary, which Greg
refers to AFAIU.

Thanks!

Re: [PATCH net v2 1/2] ptp: ocp: adjust sysfs entries to expose tty information

[Jiri Slaby]

On 14. 08. 24, 10:37, Vadim Fedorenko wrote:
> On 14/08/2024 06:00, Jiri Slaby wrote:
>> On 13. 08. 24, 20:24, Vadim Fedorenko wrote:
>>> On 13/08/2024 10:33, Greg Kroah-Hartman wrote:
>>>> On Mon, Aug 05, 2024 at 03:04:59PM -0700, Vadim Fedorenko wrote:
>>>>> Starting v6.8 the serial port subsystem changed the hierarchy of 
>>>>> devices
>>>>> and symlinks are not working anymore. Previous discussion made it 
>>>>> clear
>>>>> that the idea of symlinks for tty devices was wrong by design. 
>>>>> Implement
>>>>> additional attributes to expose the information. Fixes tag points 
>>>>> to the
>>>>> commit which introduced the change.
>>>>>
>>>>> Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be 
>>>>> children of serial core port device")
>>>>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>>>>> ---
>>>>>   drivers/ptp/ptp_ocp.c | 68 
>>>>> +++++++++++++++++++++++++++++++++----------
>>>>>   1 file changed, 52 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>>>> index ee2ced88ab34..7a5026656452 100644
>>>>> --- a/drivers/ptp/ptp_ocp.c
>>>>> +++ b/drivers/ptp/ptp_ocp.c
>>>>> @@ -3346,6 +3346,55 @@ static EXT_ATTR_RO(freq, frequency, 1);
>>>>>   static EXT_ATTR_RO(freq, frequency, 2);
>>>>>   static EXT_ATTR_RO(freq, frequency, 3);
>>>>> +static ssize_t
>>>>> +ptp_ocp_tty_show(struct device *dev, struct device_attribute 
>>>>> *attr, char *buf)
>>>>> +{
>>>>> +    struct dev_ext_attribute *ea = to_ext_attr(attr);
>>>>> +    struct ptp_ocp *bp = dev_get_drvdata(dev);
>>>>> +    struct ptp_ocp_serial_port *port;
>>>>> +
>>>>> +    port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
>>>>
>>>> That's insane pointer math, how do we know this is correct?
>>>>
>>>>> +    return sysfs_emit(buf, "ttyS%d", port->line);
>>>>> +}
>>>>> +
>>>>> +static umode_t
>>>>> +ptp_ocp_timecard_tty_is_visible(struct kobject *kobj, struct 
>>>>> attribute *attr, int n)
>>>>> +{
>>>>> +    struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
>>>>> +    struct ptp_ocp_serial_port *port;
>>>>> +    struct device_attribute *dattr;
>>>>> +    struct dev_ext_attribute *ea;
>>>>> +
>>>>> +    if (strncmp(attr->name, "tty", 3))
>>>>> +        return attr->mode;
>>>>> +
>>>>> +    dattr = container_of(attr, struct device_attribute, attr);
>>>>> +    ea = container_of(dattr, struct dev_ext_attribute, attr);
>>>>> +    port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
>>>>
>>>> That's crazy pointer math, how are you ensured that it is correct?  Why
>>>> isn't there a container_of() thing here instead?
>>>
>>> Well, container_of cannot be used here because the attributes are static
>>> while the function reads dynamic instance. The only values that are
>>> populated into the attributes of the group are offsets.
>>> But I can convert it to a helper which will check that the offset 
>>> provided is the real offset of the structure we expect. And it could 
>>> be reused in both "is_visible" and "show" functions.
>>
>> Strong NACK against this approach.
>>
>> What about converting those 4 ports into an array and adding an enum { 
>> PORT_GNSS, POTR_GNSS2, PORT_MAC, PORT_NMEA }?
> 
> Why is it a problem? I don't see big difference between these 2
> implementations:
> 
> struct ptp_ocp_serial_port *get_port(struct ptp_ocp *bp, void *offset)
> {
>      switch((uintptr_t)offset) {
>          case offsetof(struct ptp_ocp, gnss_port):
>              return &bp->gnss_port;
>          case offsetof(struct ptp_ocp, gnss2_port):
>              return &bp->gnss2_port;
>          case offsetof(struct ptp_ocp, mac_port):
>              return &bp->mac_port;
>          case offsetof(struct ptp_ocp, nmea_port):
>              return &bp->nmea_port;
>      }
>      return NULL;
> }
> 
> and:
> 
> struct ptp_ocp_serial_port *get_port(struct ptp_ocp *bp, void *offset)
> {
>      switch((enum port_type)offset) {
>          case PORT_GNSS:
>              return &bp->tty_port[PORT_GNSS];
>          case PORT_GNSS2:
>              return &bp->tty_port[PORT_GNSS2];
>          case PORT_MAC:
>              return &bp->tty_port[PORT_MAC];
>          case PORT_NMEA:
>              return &bp->tty_port[PORT_NMEA];
>      }
>      return NULL;
> }
> 
> The second option will require more LoC to change the initialization
> part of the driver, but will not simplify the access.
> If you suggest to use enum value directly, without the check, then
> it will not solve the problem of checking the boundary, which Greg
> refers to AFAIU.

Why do you need this get_port() here at all? Simply doing 
bp->tty_port[ea->var] as in already present attrs will do the job, right?

thanks,
-- 
js
suse labs

Re: [PATCH net v2 1/2] ptp: ocp: adjust sysfs entries to expose tty information

[Vadim Fedorenko]

On 14/08/2024 11:30, Jiri Slaby wrote:
> On 14. 08. 24, 10:37, Vadim Fedorenko wrote:
>> On 14/08/2024 06:00, Jiri Slaby wrote:
>>> On 13. 08. 24, 20:24, Vadim Fedorenko wrote:
>>>> On 13/08/2024 10:33, Greg Kroah-Hartman wrote:
>>>>> On Mon, Aug 05, 2024 at 03:04:59PM -0700, Vadim Fedorenko wrote:
>>>>>> Starting v6.8 the serial port subsystem changed the hierarchy of 
>>>>>> devices
>>>>>> and symlinks are not working anymore. Previous discussion made it 
>>>>>> clear
>>>>>> that the idea of symlinks for tty devices was wrong by design. 
>>>>>> Implement
>>>>>> additional attributes to expose the information. Fixes tag points 
>>>>>> to the
>>>>>> commit which introduced the change.
>>>>>>
>>>>>> Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be 
>>>>>> children of serial core port device")
>>>>>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>>>>>> ---
>>>>>>   drivers/ptp/ptp_ocp.c | 68 ++++++++++++++++++++++++++++++++ 
>>>>>> +----------
>>>>>>   1 file changed, 52 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>>>>> index ee2ced88ab34..7a5026656452 100644
>>>>>> --- a/drivers/ptp/ptp_ocp.c
>>>>>> +++ b/drivers/ptp/ptp_ocp.c
>>>>>> @@ -3346,6 +3346,55 @@ static EXT_ATTR_RO(freq, frequency, 1);
>>>>>>   static EXT_ATTR_RO(freq, frequency, 2);
>>>>>>   static EXT_ATTR_RO(freq, frequency, 3);
>>>>>> +static ssize_t
>>>>>> +ptp_ocp_tty_show(struct device *dev, struct device_attribute 
>>>>>> *attr, char *buf)
>>>>>> +{
>>>>>> +    struct dev_ext_attribute *ea = to_ext_attr(attr);
>>>>>> +    struct ptp_ocp *bp = dev_get_drvdata(dev);
>>>>>> +    struct ptp_ocp_serial_port *port;
>>>>>> +
>>>>>> +    port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
>>>>>
>>>>> That's insane pointer math, how do we know this is correct?
>>>>>
>>>>>> +    return sysfs_emit(buf, "ttyS%d", port->line);
>>>>>> +}
>>>>>> +
>>>>>> +static umode_t
>>>>>> +ptp_ocp_timecard_tty_is_visible(struct kobject *kobj, struct 
>>>>>> attribute *attr, int n)
>>>>>> +{
>>>>>> +    struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
>>>>>> +    struct ptp_ocp_serial_port *port;
>>>>>> +    struct device_attribute *dattr;
>>>>>> +    struct dev_ext_attribute *ea;
>>>>>> +
>>>>>> +    if (strncmp(attr->name, "tty", 3))
>>>>>> +        return attr->mode;
>>>>>> +
>>>>>> +    dattr = container_of(attr, struct device_attribute, attr);
>>>>>> +    ea = container_of(dattr, struct dev_ext_attribute, attr);
>>>>>> +    port = (void *)((uintptr_t)bp + (uintptr_t)ea->var);
>>>>>
>>>>> That's crazy pointer math, how are you ensured that it is correct?  
>>>>> Why
>>>>> isn't there a container_of() thing here instead?
>>>>
>>>> Well, container_of cannot be used here because the attributes are 
>>>> static
>>>> while the function reads dynamic instance. The only values that are
>>>> populated into the attributes of the group are offsets.
>>>> But I can convert it to a helper which will check that the offset 
>>>> provided is the real offset of the structure we expect. And it could 
>>>> be reused in both "is_visible" and "show" functions.
>>>
>>> Strong NACK against this approach.
>>>
>>> What about converting those 4 ports into an array and adding an enum 
>>> { PORT_GNSS, POTR_GNSS2, PORT_MAC, PORT_NMEA }?
>>
>> Why is it a problem? I don't see big difference between these 2
>> implementations:
>>
>> struct ptp_ocp_serial_port *get_port(struct ptp_ocp *bp, void *offset)
>> {
>>      switch((uintptr_t)offset) {
>>          case offsetof(struct ptp_ocp, gnss_port):
>>              return &bp->gnss_port;
>>          case offsetof(struct ptp_ocp, gnss2_port):
>>              return &bp->gnss2_port;
>>          case offsetof(struct ptp_ocp, mac_port):
>>              return &bp->mac_port;
>>          case offsetof(struct ptp_ocp, nmea_port):
>>              return &bp->nmea_port;
>>      }
>>      return NULL;
>> }
>>
>> and:
>>
>> struct ptp_ocp_serial_port *get_port(struct ptp_ocp *bp, void *offset)
>> {
>>      switch((enum port_type)offset) {
>>          case PORT_GNSS:
>>              return &bp->tty_port[PORT_GNSS];
>>          case PORT_GNSS2:
>>              return &bp->tty_port[PORT_GNSS2];
>>          case PORT_MAC:
>>              return &bp->tty_port[PORT_MAC];
>>          case PORT_NMEA:
>>              return &bp->tty_port[PORT_NMEA];
>>      }
>>      return NULL;
>> }
>>
>> The second option will require more LoC to change the initialization
>> part of the driver, but will not simplify the access.
>> If you suggest to use enum value directly, without the check, then
>> it will not solve the problem of checking the boundary, which Greg
>> refers to AFAIU.
> 
> Why do you need this get_port() here at all? Simply doing bp- 
>  >tty_port[ea->var] as in already present attrs will do the job, right?
> 

Alright, the v3 is up for review with this way applied,
thanks!

> thanks,