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

[Jens Axboe]

On 3/15/22 12:44 PM, Ondrej Zary wrote:
> On Tuesday 15 March 2022 05:22:47 Damien Le Moal wrote:
>> On 3/15/22 05:29, Jens Axboe wrote:
>>> On 3/14/22 2:25 PM, Ondrej Zary wrote:
>>>> On Monday 14 March 2022 00:19:30 Jens Axboe wrote:
>>>>> On 3/13/22 1:15 PM, Ondrej Zary wrote:
>>>>>> On Saturday 12 March 2022 15:44:15 Ondrej Zary wrote:
>>>>>>> The pata_parport is a libata-based replacement of the old PARIDE
>>>>>>> subsystem - driver for parallel port IDE devices.
>>>>>>> It uses the original paride low-level protocol drivers but does not
>>>>>>> need the high-level drivers (pd, pcd, pf, pt, pg). The IDE devices
>>>>>>> behind parallel port adapters are handled by the ATA layer.
>>>>>>>
>>>>>>> This will allow paride and its high-level drivers to be removed.
>>>>>>>
>>>>>>> paride and pata_parport are mutually exclusive because the compiled
>>>>>>> protocol drivers are incompatible.
>>>>>>>
>>>>>>> Tested with Imation SuperDisk LS-120 and HP C4381A (both use EPAT
>>>>>>> chip).
>>>>>>>
>>>>>>> Note: EPP-32 mode is buggy in EPAT - and also in all other protocol
>>>>>>> drivers - they don't handle non-multiple-of-4 block transfers
>>>>>>> correctly. This causes problems with LS-120 drive.
>>>>>>> There is also another bug in EPAT: EPP modes don't work unless a 4-bit
>>>>>>> or 8-bit mode is used first (probably some initialization missing?).
>>>>>>> Once the device is initialized, EPP works until power cycle.
>>>>>>>
>>>>>>> So after device power on, you have to:
>>>>>>> echo "parport0 epat 0" >/sys/bus/pata_parport/new_device
>>>>>>> echo pata_parport.0 >/sys/bus/pata_parport/delete_device
>>>>>>> echo "parport0 epat 4" >/sys/bus/pata_parport/new_device
>>>>>>> (autoprobe will initialize correctly as it tries the slowest modes
>>>>>>> first but you'll get the broken EPP-32 mode)
>>>>>>
>>>>>> Found a bug - the same device can be registered multiple times. Fix
>>>>>> will be in v2. But this revealed a bigger problem: pi_connect can
>>>>>> sleep (uses parport_claim_or_block) and libata does not like that. Any
>>>>>> ideas how to fix this?
>>>>>
>>>>> I think you'd need two things here:
>>>>>
>>>>> - The blk-mq queue should be registered with BLK_MQ_F_BLOCKING, which
>>>>>   will allow blocking off the queue_rq path.
>>>>
>>>> My knowledge about blk-mq is exactly zero. After grepping the code, I
>>>> guess that BLK_MQ_F_BLOCKING should be used by the block device
>>>> drivers - sd and sr?
>>>
>>> The controller would set
>>>
>>> ->needs_blocking_queue_rq = true;
>>>
>>> or something, and we'd default to false. And if that is set, when the
>>> blk-mq queue is created, then we'd set BLK_MQ_F_BLOCKING upon creation
>>> if that flag is true.
>>>
>>> That's the block layer side. Then in libata you'd need to ensure that
>>> you check that same setting and invoke ata_qc_issue() appropriately.
>>>
>>> Very top level stuff, there might be more things lurking below. But
>>> you'll probably find them as you test this stuff...
>>
>> Yes, the ata_port spinlock being held when calling ata_qc_issue() is
>> mandatory. But since I am assuming that all the IDE devices connected to
>> this adapter are QD=1 maximum, there can only be only one command in
>> flight. So it may be OK to release that lock before calling pi_connect()
>> and retake it right after it. libsas actually does something similar
>> (for no good reasons in that case though).
>>
>> Jens point remain though that since pi_connect() can sleep, marking the
>> device queue with BLK_MQ_F_BLOCKING is mandatory.
>  
> 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);
> 		pi->claimed = true;
> 		if (locked)
> 			spin_unlock(ap->lock);
> 		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.

-- 
Jens Axboe