Re: Re: [PATCH V6] serial/uart/8250: Add tunable RX interrupt trigger I/F of FIFO buffers

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

On Fri, Apr 25, 2014 at 05:53:02PM +0900, Yoshihiro YUNOMAE wrote:
> Hi Greg,
> 
> Thank you for your review.
> 
> (2014/04/25 8:11), Greg Kroah-Hartman wrote:
> >On Thu, Apr 17, 2014 at 03:06:44PM +0900, Yoshihiro YUNOMAE wrote:
> [snip]
> >>+static DEVICE_ATTR(rx_int_trig, S_IRUSR | S_IWUSR | S_IRGRP,
> >>+		   serial8250_get_attr_rx_int_trig,
> >>+		   serial8250_set_attr_rx_int_trig);
> >>+
> >
> >As you are adding a new sysfs attribute, you have to add a
> >Documentation/ABI/ entry as well.
> 
> I added this attribute to /sys/dev/char/*

What?  No.  That's not ok, why would it be?

See, Documentation would have pointed that problem out very obviously :)

> , so the documentation may be sysfs-dev. However, any other attributes
> are not written at all. Should I add this description to it or is
> there another file?

It shouldn't be on a char device, that's not acceptable.

> >>+static struct attribute *serial8250_dev_attrs[] = {
> >>+	&dev_attr_rx_int_trig.attr,
> >>+	NULL,
> >>+	};
> >>+
> >>+static struct attribute_group serial8250_dev_attr_group = {
> >>+	.attrs = serial8250_dev_attrs,
> >>+	};
> >
> >What's wrong with the macro to create a group?
> 
> I'll explain about this below.
> 
> >>+
> >>+static void register_dev_spec_attr_grp(struct uart_8250_port *up)
> >>+{
> >>+	const struct serial8250_config *conf_type = &uart_config[up->port.type];
> >>+
> >>+	if (conf_type->rxtrig_bytes[0])
> >>+		up->port.dev_spec_attr_group = &serial8250_dev_attr_group;
> >>+}
> >>+
> >>  static void serial8250_config_port(struct uart_port *port, int flags)
> >>  {
> >>  	struct uart_8250_port *up =
> >>@@ -2708,6 +2848,9 @@ static void serial8250_config_port(struct uart_port *port, int flags)
> >>  	if ((port->type == PORT_XR17V35X) ||
> >>  	   (port->type == PORT_XR17D15X))
> >>  		port->handle_irq = exar_handle_irq;
> >>+
> >>+	register_dev_spec_attr_grp(up);
> >>+	up->fcr = uart_config[up->port.type].fcr;
> >>  }
> >>
> >>  static int
> >>diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> >>index 2cf5649..41ac44b 100644
> >>--- a/drivers/tty/serial/serial_core.c
> >>+++ b/drivers/tty/serial/serial_core.c
> >>@@ -2548,15 +2548,16 @@ static struct attribute *tty_dev_attrs[] = {
> >>  	NULL,
> >>  	};
> >>
> >>-static const struct attribute_group tty_dev_attr_group = {
> >>+static struct attribute_group tty_dev_attr_group = {
> >>  	.attrs = tty_dev_attrs,
> >>  	};
> >>
> >>-static const struct attribute_group *tty_dev_attr_groups[] = {
> >>-	&tty_dev_attr_group,
> >>-	NULL
> >>-	};
> >>-
> >>+static void make_uport_attr_grps(struct uart_port *uport)
> >>+{
> >>+	uport->attr_grps[0] = &tty_dev_attr_group;
> >>+	if (uport->dev_spec_attr_group)
> >>+		uport->attr_grps[1] = uport->dev_spec_attr_group;
> >>+}
> >>
> >>  /**
> >>   *	uart_add_one_port - attach a driver-defined port structure
> >>@@ -2607,12 +2608,15 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> >>
> >>  	uart_configure_port(drv, state, uport);
> >>
> >>+	make_uport_attr_grps(uport);
> >>+
> >>  	/*
> >>  	 * Register the port whether it's detected or not.  This allows
> >>  	 * setserial to be used to alter this port's parameters.
> >>  	 */
> >>  	tty_dev = tty_port_register_device_attr(port, drv->tty_driver,
> >>-			uport->line, uport->dev, port, tty_dev_attr_groups);
> >>+			uport->line, uport->dev, port,
> >>+			(const struct attribute_group **)uport->attr_grps);
> >
> >If you have to cast that hard, something is wrong here, why are you
> >doing that?
> 
> The attribute group in serial layer was defined as constant
> because serial layer has only common sysfs I/F. However, I want to
> change sysfs I/F for specific devices. So, I deleted 'const' from the
> definition of the attribute group in serial layer in order to make the
> attribute group be changeable. On the other hand, to pass the attribute
> group to tty layer, the group must be const because the 5th variable of
> tty_port_register_device_attr() is an attribute group with 'const', so
> I implemented like this. Although I investigated again,
> tty_port_register_device_attr() is used only here, and
> tty_register_device_attr() called by the function is called from 2
> locations (the one of them passes NULL in the 5th variable).
> Therefore, we can delete 'const' for those functions, I think.
> How do you think about this?

I think you need to not be messing with the devices in /sys/dev/char/ at
all...

And why do you feel you need a sysfs attribute at all?  What is it going
to be used for?  Who needs it?  Without knowing that, I can't really
answer your questions...

greg k-h