[Linux-parport] [PATCH] pata_parport: add driver (PARIDE replacement)

[Damien Le Moal]

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.

-- 
Damien Le Moal
Western Digital Research