Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: Q0erhkoxTjXZBxNwoSbRLJZEP/SNo7iL5k2b7i2PdTZdqXGmLdKpp8yeqRz6aKykJMYCI8d+Qf
 /2jdPCwoP1mCF09zAMjIRm4IGHBs9LgE146BMbDEyZpk7DF2gkX+FAgX80DmWtUmqZ5h2UI1AO
 QypOhi/Emh/HkOOy8JxQ3LpEQ1cx71gf05I41WDd2zQB3/yRK6JgLQUNpATc+qTBr1Fpx3/IPs
 pnJ9HNta5G1TewIHRCnAbEBl5XK2TkEZhOoxDBP2YAIfHab2V34I8d+LDtBq8lim6d1jJB2z2w
 TvG1qvrOpWkMQZd2vKx/ytm6
IronPort-SDR: WPVPrXnNyXH2BCqTFcbDpIH/NWibM3NVnWKboV52GjSs238gU90asE3uj4vtGe4CAS/lWc0I9w
 hkHsezDV6ltgCqaRqIdl82W054H2Gw4uW9fMbFB4H1jPUJv/D83WIJAMkzmmpoav79qMw+8YlH
 etwVxqPFAwOlUQ3MnGj2jzp/Ask/u7Eqzhv9LsE4hlAVuxKa8fuP823/EBC8rweKP3C1JL68iO
 2JCH4tiX8Qz2lw0teSfcZgVPhgUozdaZeFD3FuEzXtAM1Hz6NZOk1eV8UpGnD3Il83RMJGPDSJ
 MK4=
WDCIronportException: Internal
Message-ID: <61c81839-ac6a-32c1-97a8-f6fefb8642d7@opensource.wdc.com>
Date:   Wed, 16 Mar 2022 09:19:22 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.2
Subject: Re: [PATCH] pata_parport: add driver (PARIDE replacement)
Content-Language: en-US
To:     Ondrej Zary <linux@zary.sk>, Jens Axboe <axboe@kernel.dk>
Cc:     Christoph Hellwig <hch@lst.de>, Tim Waugh <tim@cyberelk.net>,
        linux-block@vger.kernel.org, linux-parport@lists.infradead.org,
        linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220312144415.20010-1-linux@zary.sk>
 <202203151944.44834.linux@zary.sk>
 <1b7a45ff-31d4-4b5a-81e2-a35e7cb0b471@kernel.dk>
 <202203152217.52855.linux@zary.sk>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <202203152217.52855.linux@zary.sk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/16/22 06:17, Ondrej Zary wrote:
>>> Something like this? Requires Mike's SCSI BLK_MQ_F_BLOCKING patch:
>>> https://lore.kernel.org/all/20220308003957.123312-2-michael.christie%40oracle.com/
>>>
>>> #define PATA_PARPORT_SHT(drv_name)      \
>>>         ATA_PIO_SHT(drv_name),          \
>>>         .queuecommand_blocks    = true,
>>>
>>> static void pi_connect(struct ata_port *ap)
>>> {
>>> 	struct pi_adapter *pi = ap->host->private_data;
>>>
>>> 	del_timer_sync(&pi->timer);
>>> 	if (!pi->claimed) {
>>> 		bool locked = spin_is_locked(ap->lock);

For the pi_connect() call in the ata_qc_issue() context, ap->lock is
always held, so this is not necessary.

If you have other pi_connect() calls in different contexts, we will need
to address these too. For internal commands during scan, ap->lock is
also always held.

>>> 		pi->claimed = true;
>>> 		if (locked)
>>> 			spin_unlock(ap->lock);

You need spin_unlock_irqrestore(). See the locking done in
ata_scsi_queuecmd() which is the starting point for issuing a command
through libata.

>>> 		parport_claim_or_block(pi->pardev);
>>> 		if (locked)
>>> 			spin_lock(ap->lock);
>>> 		pi->proto->connect(pi);
>>> 	}
>>> }
>>>
>>> spin_is_locked is needed because the lock is not always held. It seems
>>> to work - no more stack traces after device double registration (only
>>> ATA errors but that's expected).
>>
>> That's a very bad paradigm. What if it is locked, but the caller isn't
>> the one that locked it? Would be better to either make the locking state
>> consistent, or provide an unlocked variant (if feasible, doesn't always
>> work if it's a provided helper already in a struct of ops), or even
>> resorting to passing in locking state as a last resort.
>  
> libata locking seems to be very complex and our functions seem to be called with various lock states. I'm lost.
> 
> Might be easier to add connect() and disconnect() to struct ata_port_operations...

But you would not be able to call these within the ata_qc_issue()
context, which I think is necessary in your case. Also, these
connect/disconnect operations are not something defined by the ATA
protocol, so we should try to keep these hidden in the LLDD. It is
better I think to find a solution about the locking, if necessary using
a different qc_issue operation or using the queuecommand_blocks
attribute to have libata call the LLDD qc_issue without lock helds,
which should be OK (need to check).

Ideally, we should refine this ap->lock big lock to avoid it being held
throughout the entire submission path. I will try to have a look at this.


-- 
Damien Le Moal
Western Digital Research