Re: [PATCH V9 1/1] usb:serial: Add Fintek F81532/534 driver

[Johan Hovold <johan@kernel.org>]

On Tue, Aug 23, 2016 at 04:23:44PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
> 
> Johan Hovold 於 2016/8/22 下午 09:14 寫道:

> >> +{
> >> +	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?

I'd say it's not worth trying to avoid that extra allocation, and there
will be several further allocations done in the usb_control_msg path
anyway. What you have today (i.e. in v9) is fine.

> >> +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.

My bad, I missed the first read above, sorry.

> >> +		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.

But you explicitly clear the xmit fifo on open it seems?

Thanks,
Johan