Re: [PATCH] pata_parport: add driver (PARIDE replacement)

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