| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <536AE8AE.2070402@hitachi.com>
Date: Thu, 08 May 2014 11:15:10 +0900
From: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@...achi.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Stephen Warren <swarren@...dia.com>,
Alan <gnomes@...rguk.ukuu.org.uk>,
Jingoo Han <jg1.han@...sung.com>, linux-kernel@...r.kernel.org,
Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>,
linux-serial@...r.kernel.org, yrl.pp-manager.tt@...achi.com,
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
Aaron Sierra <asierra@...-inc.com>, Jiri Slaby <jslaby@...e.cz>
Subject: Re: Re: Re: [PATCH V6] serial/uart/8250: Add tunable RX interrupt
trigger I/F of FIFO buffers
Hi Greg,
Sorry for the late reply.
(2014/04/26 1:01), Greg Kroah-Hartman wrote:
> 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.
My reply that I added this attribute to /sys/dev/char/* was
inappropriate. Actually, I added it to a serial device(ttyS0) in
/sys/devices/ tree. /sys/dev/char/* stores symlinks of devices
in /sys/devices/ tree.
I think the documentation where I should add the description is
Documentation/ABI/testing/sysfs-tty. (/sys/class/* also stores
symlinks of devices in /sys/devices/ tree.)
>>>> +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...
In the 1st and 2nd patches, I realized this feature by using ioctl(2),
but that was not easy to use because the RX trigger was only changeable
after opening serial fd (i.e. setting termios). Alan suggested using
sysfs I/F. I think this is better approach because
we can solve the problem easily by using existing platform of serial
devices and users can easily change the RX trigger. So, I added the
device specific sysfs attribute.
Thanks,
Yoshihiro YUNOMAE
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@...achi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists