Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
IronPort-SDR: inUCD5hJ13G6xjmJz7thmer2URS0aoM02dFHhk9psUAWgFVxJYex/airgp7FK10cGZ1va2Doiq
 GIt/w5FDK20MTDsIQKf8KiRUbAZgCJdW/mVdgG0moqSVb0V9G4HB+7mm5JfWvWn/SmNlm/YbMc
 PUvI8J50KdAD0kpyb+SBxvRMsUKCmGeuKYuWyfzdqTccb76nZyaYFC6RCjG3p6tbGaiyuDGCi9
 ZbJ7N/xERKNRam/7Tahn9giUxqnpZMnlnG9uHLIZGr1Yoc0mAPqjE2BEsi8zBubZzrZjtF1CXx
 3DckmEkx7ZAli4ZsVFUaPKAP
IronPort-SDR: yDp3SbaVi6bswYYXcquB3kHMJ5f8q/+KUKcXDwgkMWFk9cSRb9rkIvvr/2j6Jm6xBD9pl+NvEk
 gH+zBwdeX7Q9+O8p1k8pP2L7cSky+8jGaejxqN3aIntISxpxasaqNSUiP67WWisdsPxGECMvEO
 gnMuiXC+r+AFbiLDoEVXFLCVXW6jVQVbZzkU80uEBz2Oe9batI708jyOHYdSAk8Z2R0QeyGbYo
 MH148U90uumaarTnNv8abbReX61Ex2l39U59mJsCvgJXmTpS7hAKYnTBDGZCXpaPzBXOLPfywU
 4z0=
WDCIronportException: Internal
Message-ID: <8f32b2ed-13d6-c357-e417-b86a57a285db@opensource.wdc.com>
Date:   Tue, 15 Mar 2022 13:22:47 +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:     Jens Axboe <axboe@kernel.dk>, Ondrej Zary <linux@zary.sk>
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>
 <202203132015.18183.linux@zary.sk>
 <5161ed17-5f55-e851-c2e2-5340cc62fa3b@kernel.dk>
 <202203142125.40532.linux@zary.sk>
 <f8c176d4-74f0-3e4f-446f-2a5f8ace3b28@kernel.dk>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <f8c176d4-74f0-3e4f-446f-2a5f8ace3b28@kernel.dk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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