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

[Ondrej Zary]

On Wednesday 16 November 2022 02:30:46 Damien Le Moal wrote:
> On 2022/11/15 23:56, Ondrej Zary wrote:
> > On Tuesday 15 November 2022, Damien Le Moal wrote:
> >> On 11/15/22 04:25, Ondrej Zary wrote:
> >>> On Monday 14 November 2022 09:03:28 Damien Le Moal wrote:
> >>>> On 11/14/22 16:53, Ondrej Zary wrote:
> >>>>> On Monday 14 November 2022, Damien Le Moal wrote:
> >>>>>> On 11/12/22 20:17, Ondrej Zary wrote:
> >>>>>>> On Wednesday 19 October 2022 09:34:31 Christoph Hellwig wrote:
> >>>>>>>> It's been a while - did you get a chance to make some progress on
> >>>>>>>> this?  Do you need any help to unblock you?
> >>>>>>>>
> >>>>>>>
> >>>>>>> Sorry again, I'm back now. Trying to fix locking problems.
> >>>>>>> Added this to each function for analysis how the functions are called wrt.
> >>>>>>> locking:
> >>>>>>>
> >>>>>>> 	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
> >>>>>>
> >>>>>> Do you have your code somewhere that we can look at ?
> >>>>>
> >>>>> This is the current version with debug printks. I've also added dump_stack()
> >>>>> to find out the code path but haven't analyzed the output yet.
> >>>>
> >>>> Can you send a proper patch ? Or a link to a git tree ? That is easier to
> >>>> handle than pasted code in an email...
> >>>
> >>> Patch against what? I don't have a git server.
> >>
> >> patch against current 6.1-rc, or against an older kernel should be OK too.
> >> But please "git send-email" a patch, or push your dev tree to github ?
> >>
> >>> I've done some call trace analysis. These code paths are calling
> >>> pata_parport functions with ap->lock locked during init.
> >>>
> >>> Comm: kworker, Workqueue: ata_sff ata_sff_pio_task
> >>> ata_sff_hsm_move -> ata_pio_sectors-> ata_sff_altstatus -> pata_parport_tf_read -> pata_parport_check_altstatus
> >>> ata_sff_hsm_move -> ata_sff_altstatus -> pata_parport_tf_read -> pata_parport_check_altstatus
> >>> ata_sff_pio_task -> ata_sff_busy_wait -> pata_parport_check_status
> >>> ata_sff_hsm_move -> ata_wait_idle -> ata_sff_busy_wait -> pata_parport_check_status
> >>> ata_sff_hsm_move -> ata_hsm_qc_complete -> ata_sff_irq_on -> ata_wait_idle -> ata_sff_busy_wait -> pata_parport_check_status
> >>> ata_sff_pio_task -> ata_sff_hsm_move -> ata_pio_sectors -> ata_pio_sector -> ata_pio_xfer -> pata_parport_data_xfer
> >>> ata_sff_pio_task -> ata_sff_hsm_move -> pata_parport_data_xfer
> >>> ata_sff_pio_task -> ata_sff_hsm_move -> pata_parport_tf_read
> >>> ata_sff_hsm_move -> ata_hsm_qc_complete -> ata_qc_complete -> fill_result_tf -> ata_sff_qc_fill_rtf -> pata_parport_tf_read
> >>> ata_sff_hsm_move -> ata_pio_sectors -> ata_sff_altstatus -> pata_parport_check_altstatus
> >>> ata_sff_hsm_move -> ata_sff_altstatus -> pata_parport_check_altstatus
> >>>
> >>> Comm: modprobe
> >>> ata_host_start -> ata_eh_freeze_port -> ata_sff_freeze -> pata_parport_check_status
> >>>
> >>> Comm: scsi_eh_4
> >>> ata_eh_recover -> ata_eh_reset -> ata_eh_thaw_port -> ata_sff_thaw -> ata_sff_irq_on -> ata_wait_idle -> ata_sff_busy_wait -> pata_parport_check_status
> >>> ata_eh_reset -> ata_eh_freeze_port -> ata_sff_freeze -> pata_parport_check_status
> >>> ata_scsi_error -> ata_scsi_port_error_handler -> ata_port_freeze -> ata_sff_freeze -> pata_parport_check_status
> >>> ata_sff_error_handler -> pata_parport_drain_fifo -> pata_parport_check_status
> >>
> >> What exactly are the issues you are having with ap->lock ? It looks like
> >> you have done a lot of analysis of the code, but without any context about
> >> the problem, I do not understand what I am looking at.
> >>
> > 
> > The problem is that pi_connect() can sleep because it calls
> > parport_claim_or_block(). And any access (even reading ATA status register)
> > requires pi_connect.
> 
> OK. Let me have a look.
> 

The locking problems seem not to be easily solvable. Maybe a hack that grabs
the parport before registering ata interface (and keeps it until the
interface is disabled) will help? That will prevent multiple chained devices
on one parport from working but can get pata_parport moving.


-- 
Ondrej Zary