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

[Ondrej Zary]

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).

-- 
Ondrej Zary