To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-usb@vger.kernel.org
References: <20220216095153.1303105-1-mathias.nyman@linux.intel.com>
 <20220216095153.1303105-6-mathias.nyman@linux.intel.com>
 <Yg5msgveDOAaYh5u@kroah.com>
From:   Mathias Nyman <mathias.nyman@linux.intel.com>
Subject: Re: [PATCH v2 5/9] xhci: dbgtty: use IDR to support several dbc
 instances.
Message-ID: <d3da9214-d0d0-e864-cf06-4f69b64e3127@linux.intel.com>
Date:   Fri, 18 Feb 2022 12:09:41 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <Yg5msgveDOAaYh5u@kroah.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 17.2.2022 17.16, Greg KH wrote:
> On Wed, Feb 16, 2022 at 11:51:49AM +0200, Mathias Nyman wrote:
>> To support systems with several xhci controllers with active
>> dbc on each xhci we need to use IDR to identify and give
>> an index to each port.
>>
>> Avoid using global struct tty_driver.driver_state for storing
>> dbc port pointer as it won't work with several dbc ports
>>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>  drivers/usb/host/xhci-dbgcap.h |  1 +
>>  drivers/usb/host/xhci-dbgtty.c | 46 ++++++++++++++++++++++++++++------
>>  2 files changed, 40 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
>> index 5f3304a06591..ca04192fdab1 100644
>> --- a/drivers/usb/host/xhci-dbgcap.h
>> +++ b/drivers/usb/host/xhci-dbgcap.h
>> @@ -100,6 +100,7 @@ struct dbc_ep {
>>  struct dbc_port {
>>  	struct tty_port			port;
>>  	spinlock_t			port_lock;	/* port access */
>> +	int				minor;
>>  
>>  	struct list_head		read_pool;
>>  	struct list_head		read_queue;
>> diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
>> index 059b58f48e3a..d3acc0829ee5 100644
>> --- a/drivers/usb/host/xhci-dbgtty.c
>> +++ b/drivers/usb/host/xhci-dbgtty.c
>> @@ -10,11 +10,14 @@
>>  #include <linux/slab.h>
>>  #include <linux/tty.h>
>>  #include <linux/tty_flip.h>
>> +#include <linux/idr.h>
>>  
>>  #include "xhci.h"
>>  #include "xhci-dbgcap.h"
>>  
>>  static struct tty_driver *dbc_tty_driver;
>> +static struct idr dbc_tty_minors;
>> +static DEFINE_MUTEX(dbc_tty_minors_lock);
> 
> Why not just use an ida instead?  That way you do not need another lock
> and it becomes a tiny bit simpler overall.

idr seemed like a good way to tie together the minor number (in tty_struct index)
to the right dbc_port structure, which contains a pointer to that specific
xhci device 

This way I could find the right dbc_port and store it in tty_struct driver_data
in the  tty_operations install callback, allowing later tty read and and write calls
to find the correct xhci controller from tty_struct driver_data.

Looking at it now it might be possible to use just ida instead, and set the
tty_struct driver_data right after calling tty_port_register_device().

We should know everything we need there, id (minor), dbc_port, and can probably
get the tty_struct from the newly created tty port device somehow.

Should dig into tty a bit more for this.

> 
> I'll take this now, but in the future it might be worth to change this.

Thanks

-Mathias