The patch removes unused TIOCSSERIAL case from ioctl. TIOCGSERIAL case
moves to the get_serial_info() function. Some magic numbers moves to
#define directives.
Signed-off-by: Mikhail Zaytsev <[email protected]>
---
drivers/usb/serial/ark3116.c | 54 ++++++++++++++++++++++++++------------------
1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
index 23d46ef87..f45f69e18 100644
--- a/drivers/usb/serial/ark3116.c
+++ b/drivers/usb/serial/ark3116.c
@@ -36,13 +36,19 @@
#define DRIVER_DESC "USB ARK3116 serial/IrDA driver"
#define DRIVER_DEV_DESC "ARK3116 RS232/IrDA"
#define DRIVER_NAME "ark3116"
+#define ARK3116_BAUDRATE 460800
+
+#define RS232_VENDOR 0x6547
+#define RS232_PRODUCT 0x0232
+#define IRDA_VENDOR 0x18ec
+#define IRDA_PRODUCT 0x3118
/* usb timeout of 1 second */
#define ARK_TIMEOUT 1000
static const struct usb_device_id id_table[] = {
- { USB_DEVICE(0x6547, 0x0232) },
- { USB_DEVICE(0x18ec, 0x3118) }, /* USB to IrDA adapter */
+ { USB_DEVICE(RS232_VENDOR, RS232_PRODUCT) },
+ { USB_DEVICE(IRDA_VENDOR, IRDA_PRODUCT) }, /* USB to IrDA adapter */
{ },
};
MODULE_DEVICE_TABLE(usb, id_table);
@@ -50,8 +56,8 @@ MODULE_DEVICE_TABLE(usb, id_table);
static int is_irda(struct usb_serial *serial)
{
struct usb_device *dev = serial->dev;
- if (le16_to_cpu(dev->descriptor.idVendor) == 0x18ec &&
- le16_to_cpu(dev->descriptor.idProduct) == 0x3118)
+ if (le16_to_cpu(dev->descriptor.idVendor) == IRDA_VENDOR &&
+ le16_to_cpu(dev->descriptor.idProduct) == IRDA_PRODUCT)
return 1;
return 0;
}
@@ -397,31 +403,35 @@ static int ark3116_open(struct tty_struct *tty, struct usb_serial_port *port)
return result;
}
+static int ark3116_get_serial_info(struct usb_serial_port *port,
+ unsigned long arg)
+{
+ struct serial_struct ser;
+
+ memset(&ser, 0, sizeof(ser));
+
+ ser.type = PORT_16654;
+ ser.line = port->minor;
+ ser.port = port->port_number;
+ ser.custom_divisor = 0;
+ ser.baud_base = ARK3116_BAUDRATE;
+
+ if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
+ return -EFAULT;
+
+ return 0;
+}
+
static int ark3116_ioctl(struct tty_struct *tty,
unsigned int cmd, unsigned long arg)
{
struct usb_serial_port *port = tty->driver_data;
- struct serial_struct serstruct;
- void __user *user_arg = (void __user *)arg;
switch (cmd) {
case TIOCGSERIAL:
- /* XXX: Some of these values are probably wrong. */
- memset(&serstruct, 0, sizeof(serstruct));
- serstruct.type = PORT_16654;
- serstruct.line = port->minor;
- serstruct.port = port->port_number;
- serstruct.custom_divisor = 0;
- serstruct.baud_base = 460800;
-
- if (copy_to_user(user_arg, &serstruct, sizeof(serstruct)))
- return -EFAULT;
-
- return 0;
- case TIOCSSERIAL:
- if (copy_from_user(&serstruct, user_arg, sizeof(serstruct)))
- return -EFAULT;
- return 0;
+ return ark3116_get_serial_info(port, arg);
+ default:
+ break;
}
return -ENOIOCTLCMD;
--
2.11.0
Am Mittwoch, den 13.12.2017, 12:30 +0300 schrieb Mikhail Zaytsev:
> +#define RS232_VENDOR 0x6547
> +#define RS232_PRODUCT 0x0232
> +#define IRDA_VENDOR 0x18ec
> +#define IRDA_PRODUCT 0x3118
>
> /* usb timeout of 1 second */
> #define ARK_TIMEOUT 1000
>
> static const struct usb_device_id id_table[] = {
> - { USB_DEVICE(0x6547, 0x0232) },
> - { USB_DEVICE(0x18ec, 0x3118) }, /* USB to IrDA adapter */
> + { USB_DEVICE(RS232_VENDOR, RS232_PRODUCT) },
> + { USB_DEVICE(IRDA_VENDOR, IRDA_PRODUCT) }, /* USB to IrDA adapter */
Hi,
what is the purpose of this change? It just makes it harder to grep.
The constants are arbitrary and they are clearly device IDs.
Regards
Oliver
On Wed, Dec 13, 2017 at 12:30:04PM +0300, Mikhail Zaytsev wrote:
> The patch removes unused TIOCSSERIAL case from ioctl. TIOCGSERIAL case
> moves to the get_serial_info() function. Some magic numbers moves to
> #define directives.
You need to split logical changes up in separate patches, and there's at
least three things being done here.
Thanks,
Johan
On Wed, 13 Dec 2017 11:17:28 +0100 Oliver Neukum <[email protected]> wrote:
> Am Mittwoch, den 13.12.2017, 12:30 +0300 schrieb Mikhail Zaytsev:
> > +#define RS232_VENDOR 0x6547
> > +#define RS232_PRODUCT 0x0232
> > +#define IRDA_VENDOR 0x18ec
> > +#define IRDA_PRODUCT 0x3118
> >
> > /* usb timeout of 1 second */
> > #define ARK_TIMEOUT 1000
> >
> > static const struct usb_device_id id_table[] = {
> > - { USB_DEVICE(0x6547, 0x0232) },
> > - { USB_DEVICE(0x18ec, 0x3118) }, /* USB to IrDA adapter */
> > + { USB_DEVICE(RS232_VENDOR, RS232_PRODUCT) },
> > + { USB_DEVICE(IRDA_VENDOR, IRDA_PRODUCT) }, /* USB to IrDA adapter */
>
> Hi,
>
> what is the purpose of this change? It just makes it harder to grep.
> The constants are arbitrary and they are clearly device IDs.
The constants are using in several places.
I think the names easier to read.
--
Mikhail
Am Mittwoch, den 13.12.2017, 14:31 +0300 schrieb Mikhail Zaytsev:
> On Wed, 13 Dec 2017 11:17:28 +0100 Oliver Neukum <[email protected]> wrote:
>
> >
> > Am Mittwoch, den 13.12.2017, 12:30 +0300 schrieb Mikhail Zaytsev:
> > >
> > > +#define RS232_VENDOR 0x6547
> > > +#define RS232_PRODUCT 0x0232
> > > +#define IRDA_VENDOR 0x18ec
> > > +#define IRDA_PRODUCT 0x3118
> > >
> > > /* usb timeout of 1 second */
> > > #define ARK_TIMEOUT 1000
> > >
> > > static const struct usb_device_id id_table[] = {
> > > - { USB_DEVICE(0x6547, 0x0232) },
> > > - { USB_DEVICE(0x18ec, 0x3118) }, /* USB to IrDA adapter */
> > > + { USB_DEVICE(RS232_VENDOR, RS232_PRODUCT) },
> > > + { USB_DEVICE(IRDA_VENDOR, IRDA_PRODUCT) }, /* USB to IrDA adapter */
> >
> > Hi,
> >
> > what is the purpose of this change? It just makes it harder to grep.
> > The constants are arbitrary and they are clearly device IDs.
>
> The constants are using in several places.
> I think the names easier to read.
They give you nothing. If you are looking at a vendor ID nothing but the
bare number makes sense. You are just making peoples' life harder when
they have to look up that definition. A symbolic name is fine if it gives
meaning. Even if the information you give is that the value is magic
and therefore not understood. But a vendor ID is an arbitrary yet
meaningful number. There is no point in hiding it.
Regards
Oliver
On Wed, 13 Dec 2017 12:40:48 +0100 Oliver Neukum <[email protected]> wrote:
> They give you nothing. If you are looking at a vendor ID nothing but the
> bare number makes sense. You are just making peoples' life harder when
> they have to look up that definition. A symbolic name is fine if it gives
> meaning. Even if the information you give is that the value is magic
> and therefore not understood. But a vendor ID is an arbitrary yet
> meaningful number. There is no point in hiding it.
Thanks. I hear you, Oliver. What about:
- serstruct.baud_base = 460800;
Is it a magic number? I think yes.
--
Mikhail
The patch removes unused TIOCSSERIAL case from ioctl. TIOCGSERIAL case
moves to the get_serial_info() function. Some magic numbers moves to
#define directives.
Mikhail Zaytsev (2):
USB: serial: ark3116.c: Remove unused TIOCSSERIAL ioctl case.
USB: serial: ark3116.c: Move TIOCGSERIAL ioctl case to function.
drivers/usb/serial/ark3116.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
--
2.11.0
The patch moves TIOCGSERIAL ioctl case to get_serial_info function.
Signed-off-by: Mikhail Zaytsev <[email protected]>
---
drivers/usb/serial/ark3116.c | 35 +++++++++++++++++++++--------------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
index 2e957c76f..2ce8fe10e 100644
--- a/drivers/usb/serial/ark3116.c
+++ b/drivers/usb/serial/ark3116.c
@@ -36,6 +36,7 @@
#define DRIVER_DESC "USB ARK3116 serial/IrDA driver"
#define DRIVER_DEV_DESC "ARK3116 RS232/IrDA"
#define DRIVER_NAME "ark3116"
+#define ARK3116_BAUDRATE 460800
/* usb timeout of 1 second */
#define ARK_TIMEOUT 1000
@@ -397,27 +398,33 @@ static int ark3116_open(struct tty_struct *tty, struct usb_serial_port *port)
return result;
}
+static int ark3116_get_serial_info(struct usb_serial_port *port,
+ unsigned long arg)
+{
+ struct serial_struct ser;
+
+ memset(&ser, 0, sizeof(ser));
+
+ ser.type = PORT_16654;
+ ser.line = port->minor;
+ ser.port = port->port_number;
+ ser.custom_divisor = 0;
+ ser.baud_base = ARK3116_BAUDRATE;
+
+ if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
+ return -EFAULT;
+
+ return 0;
+}
+
static int ark3116_ioctl(struct tty_struct *tty,
unsigned int cmd, unsigned long arg)
{
struct usb_serial_port *port = tty->driver_data;
- struct serial_struct serstruct;
- void __user *user_arg = (void __user *)arg;
switch (cmd) {
case TIOCGSERIAL:
- /* XXX: Some of these values are probably wrong. */
- memset(&serstruct, 0, sizeof(serstruct));
- serstruct.type = PORT_16654;
- serstruct.line = port->minor;
- serstruct.port = port->port_number;
- serstruct.custom_divisor = 0;
- serstruct.baud_base = 460800;
-
- if (copy_to_user(user_arg, &serstruct, sizeof(serstruct)))
- return -EFAULT;
-
- return 0;
+ return ark3116_get_serial_info(port, arg);
default:
break;
}
--
2.11.0
The patch removes unused TIOCSSERIAL ioctl case and adds the default block
to the switch.
Signed-off-by: Mikhail Zaytsev <[email protected]>
---
drivers/usb/serial/ark3116.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
index 23d46ef87..2e957c76f 100644
--- a/drivers/usb/serial/ark3116.c
+++ b/drivers/usb/serial/ark3116.c
@@ -418,10 +418,8 @@ static int ark3116_ioctl(struct tty_struct *tty,
return -EFAULT;
return 0;
- case TIOCSSERIAL:
- if (copy_from_user(&serstruct, user_arg, sizeof(serstruct)))
- return -EFAULT;
- return 0;
+ default:
+ break;
}
return -ENOIOCTLCMD;
--
2.11.0
Am Mittwoch, den 13.12.2017, 15:30 +0300 schrieb Mikhail Zaytsev:
> On Wed, 13 Dec 2017 12:40:48 +0100 Oliver Neukum <[email protected]> wrote:
>
> >
> > They give you nothing. If you are looking at a vendor ID nothing but the
> > bare number makes sense. You are just making peoples' life harder when
> > they have to look up that definition. A symbolic name is fine if it gives
> > meaning. Even if the information you give is that the value is magic
> > and therefore not understood. But a vendor ID is an arbitrary yet
> > meaningful number. There is no point in hiding it.
>
> Thanks. I hear you, Oliver. What about:
>
> - serstruct.baud_base = 460800;
>
> Is it a magic number? I think yes.
>
Hi,
yes sure. That is a candidate for a symbolic name. Though if you use
it once, I see no benefit, but it does not hurt either. The member
is named and that is the important thing.
A line like
if (rate > 38400) return -EINVAL;
is not so good
if (rate > MAX_BAUD) return -EINVAL;
better
But:
device->maxbaudrate = 38400
is better than
device->maxbaudrate = MAX_BAUD
You see the point?
Regards
Oliver
On Wed, 13 Dec 2017 15:39:21 +0100 Oliver Neukum <[email protected]> wrote:
> But:
>
> device->maxbaudrate = 38400
>
> is better than
>
> device->maxbaudrate = MAX_BAUD
>
> You see the point?
Yes, I see. This is better, because it's more important to know =<how many>, but not =<what>.
thank you Oliver.
---
Mikhail
On Wed, Dec 13, 2017 at 04:44:55PM +0300, Mikhail Zaytsev wrote:
> The patch removes unused TIOCSSERIAL ioctl case and adds the default block
> to the switch.
>
> Signed-off-by: Mikhail Zaytsev <[email protected]>
> ---
> drivers/usb/serial/ark3116.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
> index 23d46ef87..2e957c76f 100644
> --- a/drivers/usb/serial/ark3116.c
> +++ b/drivers/usb/serial/ark3116.c
> @@ -418,10 +418,8 @@ static int ark3116_ioctl(struct tty_struct *tty,
> return -EFAULT;
>
> return 0;
> - case TIOCSSERIAL:
> - if (copy_from_user(&serstruct, user_arg, sizeof(serstruct)))
> - return -EFAULT;
> - return 0;
> + default:
> + break;
> }
>
> return -ENOIOCTLCMD;
This will make the ioctl return -ENOTTY to user space (e.g. setserial),
which I guess should be fine as TIOCSSERIAL really isn't supported for
these devices currently.
But you should at least mention this changed behaviour in the commit
message.
Please also drop the ".c" part from the subject prefix while at it.
Thanks,
Johan
On Wed, Dec 13, 2017 at 04:45:06PM +0300, Mikhail Zaytsev wrote:
> The patch moves TIOCGSERIAL ioctl case to get_serial_info function.
>
> Signed-off-by: Mikhail Zaytsev <[email protected]>
> ---
> drivers/usb/serial/ark3116.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
> index 2e957c76f..2ce8fe10e 100644
> --- a/drivers/usb/serial/ark3116.c
> +++ b/drivers/usb/serial/ark3116.c
> @@ -36,6 +36,7 @@
> #define DRIVER_DESC "USB ARK3116 serial/IrDA driver"
> #define DRIVER_DEV_DESC "ARK3116 RS232/IrDA"
> #define DRIVER_NAME "ark3116"
> +#define ARK3116_BAUDRATE 460800
I thought you and Oliver agreed this wasn't needed (wanted). Just
hard-code this number in the new helper as before.
> /* usb timeout of 1 second */
> #define ARK_TIMEOUT 1000
> @@ -397,27 +398,33 @@ static int ark3116_open(struct tty_struct *tty, struct usb_serial_port *port)
> return result;
> }
>
> +static int ark3116_get_serial_info(struct usb_serial_port *port,
> + unsigned long arg)
Keep the (void __user *) cast in ark3116_ioctl below and pass a struct
serial_struct __user pointer here.
> +{
> + struct serial_struct ser;
> +
> + memset(&ser, 0, sizeof(ser));
> +
> + ser.type = PORT_16654;
> + ser.line = port->minor;
> + ser.port = port->port_number;
> + ser.custom_divisor = 0;
Might as well drop this redundant assignment as well.
> + ser.baud_base = ARK3116_BAUDRATE;
> +
> + if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> static int ark3116_ioctl(struct tty_struct *tty,
> unsigned int cmd, unsigned long arg)
> {
> struct usb_serial_port *port = tty->driver_data;
> - struct serial_struct serstruct;
> - void __user *user_arg = (void __user *)arg;
>
> switch (cmd) {
> case TIOCGSERIAL:
> - /* XXX: Some of these values are probably wrong. */
> - memset(&serstruct, 0, sizeof(serstruct));
> - serstruct.type = PORT_16654;
> - serstruct.line = port->minor;
> - serstruct.port = port->port_number;
> - serstruct.custom_divisor = 0;
> - serstruct.baud_base = 460800;
> -
> - if (copy_to_user(user_arg, &serstruct, sizeof(serstruct)))
> - return -EFAULT;
> -
> - return 0;
> + return ark3116_get_serial_info(port, arg);
> default:
> break;
> }
Thanks,
Johan