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

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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.

-- 
Damien Le Moal
Western Digital Research