On Wed, Oct 27, 2021 at 03:28:42PM +0200, Himadri Pandya wrote:

On Wed, Oct 27, 2021 at 3:04 PM Johan Hovold [off-list ref] wrote:

quoted

On Fri, Oct 01, 2021 at 08:57:19AM +0200, Himadri Pandya wrote:

quoted

quoted

@@ -287,23 +277,18 @@ static int ch341_set_handshake(struct usb_device *dev, u8 control)
 static int ch341_get_status(struct usb_device *dev, struct ch341_private *priv)
 {
      const unsigned int size = 2;
-     char *buffer;
+     u8 buffer[2];
      int r;
      unsigned long flags;

-     buffer = kmalloc(size, GFP_KERNEL);
-     if (!buffer)
-             return -ENOMEM;
-
      r = ch341_control_in(dev, CH341_REQ_READ_REG, 0x0706, 0, buffer, size);
-     if (r < 0)
-             goto out;
+     if (r)
+             return r;

      spin_lock_irqsave(&priv->lock, flags);
      priv->msr = (~(*buffer)) & CH341_BITS_MODEM_STAT;
      spin_unlock_irqrestore(&priv->lock, flags);

-out: kfree(buffer);
      return r;

This should now be

        return 0;

Yes. The function was returning the negative error value before the
change. But now it doesn't need to as we are already taking care of it
in the wrapper.

It has more to do with the fact that we now return early on errors so r
will always be zero here. It's better to be explicit about that.
 

quoted

quoted

 }

@@ -312,30 +297,25 @@ out:    kfree(buffer);
 static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
 {
      const unsigned int size = 2;
-     char *buffer;
+     u8 buffer[2];
      int r;

-     buffer = kmalloc(size, GFP_KERNEL);
-     if (!buffer)
-             return -ENOMEM;
-
      /* expect two bytes 0x27 0x00 */
      r = ch341_control_in(dev, CH341_REQ_READ_VERSION, 0, 0, buffer, size);
-     if (r < 0)
-             goto out;
+     if (r)
+             return r;
      dev_dbg(&dev->dev, "Chip version: 0x%02x\n", buffer[0]);

      r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT, 0, 0);
-     if (r < 0)
-             goto out;
+     if (r)
+             return r;

Now an unrelated change.

I think it is a related change because we are removing the out label.

Sorry, I meant that the (r < 0) change was unrelated since you're no
longer touching ch341_control_out(). The return is indeed still needed.
 

quoted

quoted

@@ -647,23 +611,19 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
      struct ch341_private *priv = usb_get_serial_port_data(port);
      int r;
      uint16_t reg_contents;
-     uint8_t *break_reg;
+     uint8_t break_reg[2];

      if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
              ch341_simulate_break(tty, break_state);
              return;
      }

-     break_reg = kmalloc(2, GFP_KERNEL);
-     if (!break_reg)
-             return;
-
      r = ch341_control_in(port->serial->dev, CH341_REQ_READ_REG,
                      ch341_break_reg, 0, break_reg, 2);
-     if (r < 0) {
+     if (r) {
              dev_err(&port->dev, "%s - USB control read error (%d)\n",
                              __func__, r);
-             goto out;
+             return;
      }
      dev_dbg(&port->dev, "%s - initial ch341 break register contents - reg1: %x, reg2: %x\n",
              __func__, break_reg[0], break_reg[1]);

@@ -681,11 +641,9 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
      reg_contents = get_unaligned_le16(break_reg);
      r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
                      ch341_break_reg, reg_contents);
-     if (r < 0)
+     if (r)

Now also an unrelated change.

Maybe I misunderstood your comments on v2. I thought you asked to get
rid of the out labels in callers.

Yes, but as above I'm referring to the (r < 0) change for
ch341_control_out() which is now unrelated to the rest of the patch.

Johan