Message-ID: <d1a32b2f-c90a-69f6-3063-f0babd2a020a@gmail.com>
Date:   Tue, 23 Aug 2016 16:23:44 +0800
From:   "Ji-Ze Hong (Peter Hong)" <hpeter@...il.com>
To:     Johan Hovold <johan@...nel.org>, gregkh@...uxfoundation.org,
        Peter Hung <hpeter@...il.com>,
        One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
Cc:     tom_tsai@...tek.com.tw, peter_hong@...tek.com.tw,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V9 1/1] usb:serial: Add Fintek F81532/534 driver

Hi Johan,

Johan Hovold 於 2016/8/22 下午 09:14 寫道:

>> +static const struct reg_value f81534_pin_control[4][3] = {
>> +	/* M0_SD	M1		M2 */
>> +	{{0x2ae8, 7}, {0x2a90, 5}, {0x2a90, 4}, },	/* port 0 pins */
>> +	{{0x2ae8, 6}, {0x2ae8, 0}, {0x2ae8, 3}, },	/* port 1 pins */
>> +	{{0x2a90, 0}, {0x2ae8, 2}, {0x2a80, 6}, },	/* port 2 pins */
>> +	{{0x2a90, 3}, {0x2a90, 2}, {0x2a90, 1}, },	/* port 3 pins */
>> +};
>
> I thought we agreed to drop the transceiver configuration from the
> driver in favour of a user-space tool?

OK, I'll remove all transceiver setting next version.


>> +{
>> +	size_t count = F81534_USB_MAX_RETRY;
>> +	int status;
>> +	u8 *tmp;
>> +
>> +	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
>> +	if (!tmp)
>> +		return -ENOMEM;
>> +
>> +	*tmp = data;
>> +
>> +	/*
>> +	 * Our device maybe not reply when heavily loading, We'll retry for
>> +	 * F81534_USB_MAX_RETRY times.
>> +	 */
>> +	while (count--) {
>> +		status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
>> +					 F81534_SET_GET_REGISTER,
>> +					 USB_TYPE_VENDOR | USB_DIR_OUT,
>> +					 reg, 0, tmp, sizeof(u8),
>> +					 F81534_USB_TIMEOUT);
>> +		if (status > 0)
>> +			break;
>> +
>> +		if (status == 0)
>> +			status = -EIO;
>> +	}
>> +
>> +	if (status < 0) {
>> +		dev_err(&dev->dev, "%s: reg: %x data: %x failed: %d\n",
>> +				__func__, reg, data, status);
>> +		kfree(tmp);
>> +		return status;
>
> I'd use a common exit path to free tmp, and just print an error here.

I'll change this with next patch. BTW, Alan had suggested me to re-write
set/get register function to avoid kmalloc(), but I found some issue
to re-write.

We need to read the internal storage to determinate the port counts in
f81534_calc_num_ports(), but in this moment the usb_serial had no
private data, it still need to use kmalloc() to get memory.

The following source code is my current modification. I'll kmalloc
the buffer if it can't get serial_private, otherwise I'll use
serial_private buffer and protected by a mutex. Should I do something
to improve it?

static int f81534_set_normal_register(struct usb_serial *serial, u16 reg,
					u8 data)
{
	struct f81534_serial_private *serial_priv =
			usb_get_serial_data(serial);
	struct usb_interface *interface = serial->interface;
	struct usb_device *dev = serial->dev;
	size_t count = F81534_USB_MAX_RETRY;
	int status;
	u8 *tmp;

	/*
	 * The f81534_set_normal_register can be called by
	 * f81534_calc_num_ports(). It'll run before allocate private data of
	 * usb_serial. So we should treat as a special case.
	 */
	if (serial_priv) {
		mutex_lock(&serial_priv->register_mutex);
		tmp = &serial_priv->reg_value;
	} else {
		tmp = kmalloc(sizeof(u8), GFP_KERNEL);
		if (!tmp)
			return -ENOMEM;
	}

	*tmp = data;

	/*
	 * Our device maybe not reply when heavily loading, We'll retry for
	 * F81534_USB_MAX_RETRY times.
	 */
	while (count--) {
		status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
					 F81534_SET_GET_REGISTER,
					 USB_TYPE_VENDOR | USB_DIR_OUT,
					 reg, 0, tmp, sizeof(u8),
					 F81534_USB_TIMEOUT);
		if (status > 0) {
			status = 0;
			break;
		} else if (status == 0) {
			status = -EIO;
		}
	}

	if (status < 0) {
		dev_err(&interface->dev, "%s: reg: %x data: %x failed: %d\n",
				__func__, reg, data, status);
	}

	if (serial_priv)
		mutex_unlock(&serial_priv->register_mutex);
	else
		kfree(tmp);

	return status;
}



>>
>> +static int f81534_command_delay(struct usb_serial *usbserial)
>
> Please explain why and when you need to use this "delay" function, and
> how the BUS_REG_STATUS register works.
>
> Please use "serial" consistently throughout for usb_serial pointers
> (instead of "usbserial").

OK, I'll add comment above f81534_command_delay() and change all
usbserial to serial.


>> +static int f81534_calc_num_ports(struct usb_serial *serial)
>> +{
>> +	unsigned char setting[F81534_CUSTOM_DATA_SIZE];
>> +	uintptr_t setting_idx;
>> +	u8 num_port = 0;
>> +	int status;
>> +	size_t i;
>> +
>> +	/* Check had custom setting */
>> +	status = f81534_find_config_idx(serial, &setting_idx);
>> +	if (status) {
>> +		dev_err(&serial->dev->dev, "%s: find idx failed: %d\n",
>> +				__func__, status);
>> +		return 0;
>> +	}
>> +
>> +	/* Save the configuration area idx as private data for attach() */
>> +	usb_set_serial_data(serial, (void *)setting_idx);
>> +
>> +	/* Read default board setting */
>> +	status = f81534_read_data(serial, F81534_DEF_CONF_ADDRESS_START,
>> +				  F81534_NUM_PORT, setting);
>> +	if (status) {
>> +		dev_err(&serial->dev->dev, "%s: read failed: %d\n", __func__,
>> +				status);
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * If had custom setting, override it. 1st byte is a indicator, 0xff
>> +	 * is empty, F81534_CUSTOM_VALID_TOKEN is had data, then skip with 1st
>> +	 * data
>> +	 */
>> +	if (setting_idx != F81534_CUSTOM_NO_CUSTOM_DATA) {
>> +		status = f81534_read_data(serial, F81534_CUSTOM_ADDRESS_START +
>> +						F81534_CONF_OFFSET,
>> +						sizeof(setting), setting);
>> +		if (status) {
>> +			dev_err(&serial->dev->dev,
>> +					"%s: get custom data failed: %d\n",
>> +					__func__, status);
>> +			return 0;
>> +		}
>> +
>> +		dev_dbg(&serial->dev->dev,
>> +				"%s: read configure from block: %d\n",
>> +				__func__, (unsigned int)setting_idx);
>> +	} else {
>> +		dev_dbg(&serial->dev->dev, "%s: read configure default\n",
>> +				__func__);
>> +	}
>> +
>> +	/* New style, find all possible ports */
>> +	num_port = 0;
>> +	for (i = 0; i < F81534_NUM_PORT; ++i) {
>> +		if (setting[i] & F81534_PORT_UNAVAILABLE)
>> +			continue;
>
> Looks like setting[] could be uninitialised here.

Our IC will preserve 2 section for configuration data. One is
F81534_DEF_CONF_ADDRESS_START, another is F81534_CUSTOM_ADDRESS_START.

We'll read F81534_DEF_CONF_ADDRESS_START first for default value and
read F81534_CUSTOM_ADDRESS_START for customer value.


>
>> +	size_t i, read_size = 0;
>> +	unsigned long flags;
>> +	bool available;
>> +	char tty_flag;
>> +	int status;
>> +	u8 lsr;
>> +
>> +	available = !!atomic_read(&priv->port_active[phy_port_num]);
>> +
>> +	/*
>> +	 * The block layout is 128 Bytes
>> +	 * index 0: port phy idx (e.g., 0,1,2,3),
>> +	 * index 1: It's could be
>> +	 *			F81534_TOKEN_RECEIVE
>> +	 *			F81534_TOKEN_TX_EMPTY
>> +	 *			F81534_TOKEN_MSR_CHANGE
>> +	 * index 2: serial in size (data+lsr, must be even)
>> +	 *			meaningful for F81534_TOKEN_RECEIVE only
>> +	 * index 3: current MSR with this device
>> +	 * index 4~127: serial in data block (data+lsr, must be even)
>> +	 */
>> +	switch (data[1]) {
>> +	case F81534_TOKEN_TX_EMPTY:
>> +		/*
>> +		 * We should save TX_EMPTY flag even the port is not opened
>> +		 */
>> +		spin_lock_irqsave(&priv->tx_empty_lock, flags);
>> +		priv->is_phy_port_not_empty[phy_port_num] = false;
>> +		spin_unlock_irqrestore(&priv->tx_empty_lock, flags);
>
> Why not just keep a flag in the port private data?
>
> Also could this end up racing with f81534_submit_writer() which could
> have just set this flag?

I'll change TX empty flag from serial_private to every port_private
next patch.

We can send TX data only when serial port is reported with tx empty
state. So it can't be racing with H/W in normal state.


>
>> +		tty_port_num = f81534_phy_to_logic_port(serial, phy_port_num);
>> +		port = serial->port[tty_port_num];
>> +
>> +		/*
>> +		 * The device will send back all information when we submitted
>> +		 * a read URB (MSR/DATA/TX_EMPTY). But it maybe get callback
>> +		 * before port_probe() or after port_remove().
>> +		 *
>> +		 * So we'll verify the pointer. If the pointer is NULL, it's
>> +		 * mean the port not init complete and the block will skip.
>> +		 */
>> +		port_priv = usb_get_serial_port_data(port);
>
> Check if the port has been opened here instead, no need to store MSR for
> an unused port above.

It's useless for MSR & Receive data when port is closed, but we need
the URB to receive TX empty flag. We may not received TX empty flag
if we don't process when port is closed. It'll make the port not
workable.

So this section need to be preserved.


>> +		if (!port_priv) {
>> +			dev_warn(&serial->dev->dev,
>> +					"%s: phy: %d not ready\n", __func__,
>> +					phy_port_num);
>> +			continue;
>> +		}
>> +
>> +		f81534_process_per_serial_block(port, &ch[i]);
>
> Missing sanity check on size of the received data, which you access
> unconditionally in the helper function.

The H/W will report URB data length with 1x/2x/3x/4x
F81534_RECEIVE_BLOCK_SIZE. I'll add sanity check likes:

if (!urb->actual_length ||
	(urb->actual_length % F81534_RECEIVE_BLOCK_SIZE))
	return;

above the f81534_process_per_serial_block() to make sure the packet
length is acceptable.


>> +	if (status) {
>> +		dev_err(&serial->dev->dev,
>> +				"%s: idx: %d get data failed: %d\n", __func__,
>> +				serial_priv->setting_idx, status);
>> +		return status;
>> +	}
>> +
>> +	/*
>> +	 * We'll register port 0 bulkin only once, It'll take all port received
>> +	 * data, MSR register change and TX_EMPTY information.
>> +	 */
>> +	status = f81534_submit_read_urb(serial, GFP_KERNEL);
>> +	if (status)
>> +		return status;
>> +
>> +	return 0;
>> +}
>
> You need to stop the read urbs you submitted in attach in a matching
> release() callback.
>
> But as I've mentioned before, you should consider keeping an open-port
> count and submit the reads urbs when the first port is opened and stop
> them when the last port is closed instead.

I'll add release() to stop port0 read URB. We can't stop read URB when
final port close due to TX empty issue. It's only cant be received by
read URB and it could be lost when H/W read URB memory is full or
flushed. So we need to use permanent read URB to get port tx empty
status.


>> +static int f81534_tiocmget(struct tty_struct *tty)
>> +{
>> +	struct usb_serial_port *port = tty->driver_data;
>> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
>> +	unsigned long flags;
>> +	int r;
>> +	u8 msr, mcr;
>> +
>> +	/*
>> +	 * We'll avoid to direct read MSR register without open(). The IC will
>> +	 * read the MSR changed and report it f81534_process_per_serial_block()
>> +	 * by F81534_TOKEN_MSR_CHANGE.
>
> Why not read it directly from the chip if you can? This will never be
> called for a closed port.

I'll try to directly read register in tiocmget().

Thanks for your help.
-- 
With Best Regards,
Peter Hong

Powered by blists - more mailing lists