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
