From: Jarkko Sonninen > Sent: 13 March 2023 08:28 > > Add support for RS-485 in Exar USB adapters. > RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls. > Gpio mode register is set to enable RS-485. The locking is entirely dubious. Summary: Taking the lock to read the flags is pretty pointless. You are only looking at one bit and nothing else is tied to the lock. Even a READ_ONCE() isn't needed. > + spin_lock_irqsave(&data->lock, flags); > + rs485_flags = data->rs485.flags; > + spin_unlock_irqrestore(&data->lock, flags); > + if (rs485_flags & SER_RS485_ENABLED) > + gpio_mode |= XR_GPIO_MODE_SEL_RS485 | XR_GPIO_MODE_RS485_TX_H; > + else if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) > + gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS; > + The ioctl read code reads the data unlocked. > + if (copy_to_user(argp, &data->rs485, sizeof(data->rs485))) > + return -EFAULT; So could return old and new data if the ioctl write code runs concurrently (and you get a hardware interrupt or page fault mid-buffer). The ioctl write code acquires the lock across a structure copy. (which should be a structure copy, not a memcpy). The only way the lock will have any effect is if multiple threads are doing updates at the same time. Code doing that won't work anyway. > + if (copy_from_user(&rs485, argp, sizeof(rs485))) > + return -EFAULT; > + > + dev_dbg(tty->dev, "Flags %02x\n", rs485.flags); > + rs485.flags &= SER_RS485_ENABLED; > + spin_lock_irqsave(&data->lock, flags); > + memcpy(&data->rs485, &rs485, sizeof(rs485)); > + spin_unlock_irqrestore(&data->lock, flags); In any case you one seem to be implementing one bit of the flags - so the rest of the data can be ignored. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
