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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
