Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: hQJjdZkmxhQZeWId88dLUjMLh+g/OFTlqqmFWJ7q36PVmzQFT+UwIP0FOanbVulVTYRdbYOlSc
 oNHkFsPZR9/o4sdgXu48fYou4zrlsMQvSNFXVBIUOMoooqUeizQChRwNAE1VlGEFrNqLgv8Son
 GV67Tehjgs72uOiBTX6PLzaskk+V0F1M2LYlWPJF7dasVOeCVH0UNy+Db5Zb4lJpiQllQyy0lz
 0SsM8sIlnU1/EKt2BfY549e4hZEPfcswVWDGWv8FivKqoIp2VyYqKhfzZ44aUwgP92u59Ht1qD
 bx0=
IronPort-SDR: 93UsCCsDb8KnP39Oo1sS+E6qaV69Gy72m+k5JVrIwF2Zj3kc0U4LcbGUG3dONMqNZS4YyIV2vH
 qRBKq+TWzswSvpVf6uJ4FetIZ0YnbWhk1PTEFaE5nriuR25yO9MRYg2duzIp1OW5H3UAPclS12
 eQ1zWWTYXpZ4avnnNKTMRVakUwxuUGymb/YEUNQdGW+RqsY/xwW7jcFNaFnvlMPAFEJdE8N1l9
 qU0DV75LCrlRTPKmpa7Y2kTivi5aUyKxs/3cIawrAZbQK8D+vmI03GNrl5B12lK3h6bH3+ERrP
 q1M=
WDCIronportException: Internal
Message-ID: <dcd31bf2-5a7a-30a4-4c3c-205d5f39fcd8@opensource.wdc.com>
Date:   Tue, 13 Dec 2022 08:07:31 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH] pata_parport: add driver (PARIDE replacement)
Content-Language: en-US
To:     Ondrej Zary <linux@zary.sk>
Cc:     Christoph Hellwig <hch@lst.de>,
        Sergey Shtylyov <s.shtylyov@omp.ru>,
        Jens Axboe <axboe@kernel.dk>, 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>
 <202211151556.52895.linux@zary.sk>
 <bfc49618-dbc3-8ffb-f536-3b4486e1980e@opensource.wdc.com>
 <202212122355.30988.linux@zary.sk>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <202212122355.30988.linux@zary.sk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/13/22 07:55, Ondrej Zary wrote:
> 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.

I have been very busy and had no time to look at this. Sorry about that. I
will have a look before year-end break.

-- 
Damien Le Moal
Western Digital Research