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
