Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RESEND PATCH V3 1/3] USB: serial: f81232: clear overrun flag
To:     Oliver Neukum <oneukum@suse.com>, peter_hong@fintek.com.tw,
        johan@kernel.org, gregkh@linuxfoundation.org
Cc:     "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@gmail.com>,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
References: <1554098453-31278-1-git-send-email-hpeter+linux_kernel@gmail.com>
 <1554107689.6310.17.camel@suse.com>
From:   "Ji-Ze Hong (Peter Hong)" <hpeter@gmail.com>
Message-ID: <fa56bb5b-8945-29f1-8516-58ce0c8ebd39@gmail.com>
Date:   Mon, 1 Apr 2019 17:01:29 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <1554107689.6310.17.camel@suse.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Oliver Neukum 於 2019/4/1 下午 04:34 寫道:
> On Mo, 2019-04-01 at 14:00 +0800,  Ji-Ze Hong (Peter Hong)  wrote:
> 
> Hi,
> I am afraid there is a race condiion in this code.
> 
> 
>> @@ -315,6 +318,7 @@ static void f81232_process_read_urb(struct urb *urb)
>>   
>>   			if (lsr & UART_LSR_OE) {
>>   				port->icount.overrun++;
>> +				schedule_work(&priv->lsr_work);
> 
> Unconditionally scheduled
> 
>>   				tty_insert_flip_char(&port->port, 0,
>>   						TTY_OVERRUN);
>>   			}
> 
> [..]
>> +static int f81232_suspend(struct usb_serial *serial, pm_message_t message)
>> +{
>> +	struct f81232_private *port_priv;
>> +
>> +	port_priv = usb_get_serial_port_data(serial->port[0]);
>> +	flush_work(&port_priv->lsr_work);
>> +
>> +	return 0;
>> +}
>> +
>>   static struct usb_serial_driver f81232_device = {
>>   	.driver = {
>>   		.owner =	THIS_MODULE,
>> @@ -655,6 +688,7 @@ static struct usb_serial_driver f81232_device = {
>>   	.read_int_callback =	f81232_read_int_callback,
>>   	.port_probe =		f81232_port_probe,
>>   	.port_remove =		f81232_port_remove,
>> +	.suspend =		f81232_suspend,
> 
> Please have a look at:
> int usb_serial_suspend(struct usb_interface *intf, pm_message_t message)
> {
>          struct usb_serial *serial = usb_get_intfdata(intf);
>          int i, r = 0;
> 
> 
>          serial->suspending = 1;
> 
> 
>          /*
>           * serial->type->suspend() MUST return 0 in system sleep context,
>           * otherwise, the resume callback has to recover device from
>           * previous suspend failure.
>           */
>          if (serial->type->suspend) {
>                  r = serial->type->suspend(serial, message);
>                  if (r < 0) {
>                          serial->suspending = 0;
>                          goto err_out;
>                  }
>          }
> 
> 
>          for (i = 0; i < serial->num_ports; ++i)
>                  usb_serial_port_poison_urbs(serial->port[i]);
> err_out:
>          return r;
> }
> EXPORT_SYMBOL(usb_serial_suspend);
> 
> As you can see, the suspend method is called first and then the URBs
> are poisoned. That means that after you have flushed the work, it may
> be submitted again. The fix would be to test the 'suspending' flag
> before you schedule work (and recheck the need to schedule it
> during resume)

Thanks for report the race condition issue. It's seems the same bug
in f81534.c. I'll try to fix it on f81232.c then fix f81534.c too.

-- 
With Best Regards,
Peter Hong