Message-ID: <0804e457-27ae-a046-a3f5-73497511c841@nvidia.com>
Date:   Tue, 31 Jan 2023 11:06:21 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH 1/5] genirq: Use hlist for managing resend handlers
Content-Language: en-US
From:   Shanker Donthineni <sdonthineni@nvidia.com>
To:     Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>, Michael Walle <michael@walle.cc>
Cc:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Hans de Goede <hdegoede@redhat.com>,
        Wolfram Sang <wsa+renesas@sang-engineering.com>,
        linux-kernel@vger.kernel.org
References: <20230130005725.3517597-1-sdonthineni@nvidia.com>
 <20230130005725.3517597-2-sdonthineni@nvidia.com> <87pmav13ly.ffs@tglx>
 <182e3214-0f60-8b65-5981-23d2a2ea54b9@nvidia.com>
In-Reply-To: <182e3214-0f60-8b65-5981-23d2a2ea54b9@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dUVwK2h1dkJ4c0l3c1N3S0x4K3hUUnd1WEQwTUQxV1RuZ0x4cE5Bb0RSZnhu?=
 =?utf-8?B?SzMvYjFBbG5aaDBiSDhjb21PK05NM3JwaEVnNzFabDBrb3Ivb243ZWs5Kzdh?=
 =?utf-8?B?WXV3N2xmU1NxQlgrOWZSMnE1QVJITVh3aFp0SjgraS9TaDV5WnVabitwaWU3?=
 =?utf-8?B?Q3dNVEpNeU5vVEpsZy9BeXhCVVBPdEpycVFXZ2JiTHlQN2RCbk5adVFJL0Zo?=
 =?utf-8?B?Z2RUN2V5Y09pNEM2cDk3TjVlSmdoRzFZSUpUUVprWURqZDdFK3ArSFMwL2tt?=
 =?utf-8?B?VFE0Vi9xUm5BOEc2TklVOFhiUXU5dXZQNklSd2xUWGxpdjVtekdMY2xpRXpx?=
 =?utf-8?B?eVNvZW90TnJiTVlXV0U3NGNxY0JRN1k5OXpsMWVTWVdsVnlvMGYwdjlHbTdt?=
 =?utf-8?B?YS9kV0lxMWVJenEvS1BKNXlBYXBTdmlNYXdZVEwrVVZ3d3BsZTlLZTFLdWlw?=
 =?utf-8?B?cTBpRjY5RkFuRnZtRWhBM1FjbXhuY2VDVlVQSWF0cTlKZ3hwWkpNWGpaRWpL?=
 =?utf-8?B?RXZ3Nm1pYzlaNzQwTVE2TVFnRkkrYlYxY2dQTEFNVFVoTVZWUWpXNXVTQ052?=
 =?utf-8?B?QkJVODBHM2c4V00rUFV5dk11SWVvN21iaGIxS0JmU1Yrb24zZGVZakJIVG8y?=
 =?utf-8?B?N1ZOV3h5UDk1bG5TRHVsTHlWM0krVE54cDViTkdDY3g2Zk9yYS91aXdFbVow?=
 =?utf-8?B?U1pJZFdNcWxoRVlFQy83MEp5dWtHSHNObWkrMTBrM2pTdHNIT3RmT0xXWDhQ?=
 =?utf-8?B?UXhoT3IrZFVJUjFqR3hmdzNRbVYwTTRUZ3d1eCtKa08wYTN0WlhKOGJ3R2cy?=
 =?utf-8?B?YS9pTjh1aEVjbk5ONzRBTFlYaHk5cXExUGZtZ2lQMHZTUklSZ3NHYWt4b3J3?=
 =?utf-8?B?ejV1bCtXRThuOGV6c0NuTGZwYTIzK2V6cEVTeXkvenBXUHBCVVRmYVJrN0Qy?=
 =?utf-8?B?cEZzWEV0c0xDUW54VWZuL3IvUzhRdFJqK0xpOG5yU21CMUpzZEs1amFSd05i?=
 =?utf-8?B?b3U0R2ZJLzhNUGNKZU5KVDQ5TzBFMzdIbDUxanNPKy9GdjZ3Ny9CaEdGTEJw?=
 =?utf-8?B?eXY5dlArbjB2Ym9QNWF2SzRuYnNITy9xdTlaVUoxa29xa08ydnR5SXJSS2tY?=
 =?utf-8?B?VG53a1MwN1NsdFNjRm1VaE9pVlJpanM3N00xSkVwZDNFRHJXYTBicE1Dc1dt?=
 =?utf-8?B?WUdvdW1JNmd2WkdsV1kySXNYZjRxOWFmWVJjV1FwVHlNd05GVmdCeGdkOUg4?=
 =?utf-8?B?Q3hLZ0h1czJzckQ5MDg5Z3F0a0l4K0VmMmNKRi91ejZ2R0dzTWxnMWhqcDI2?=
 =?utf-8?B?dmNTYjREZS81YmpWWkNZdGNZT2poaExsS0xHazZ1OHZnV29GNG5DeEZLTUNQ?=
 =?utf-8?B?QzBoLy83anNjYld2bVNFNXRsNHhCZDhUUmY5TytxVm13NkhYZHIvamZOcGNK?=
 =?utf-8?B?STlVWm5ENDI0M0wxeSt2OTZ4SHRlcDRzUmRLeXJ1c0VQb3BRK20rMlNDOHl5?=
 =?utf-8?B?bGdoTlBjcUZLYkI3ajhPdUZ1NjZHc0Y1eG9LVmZUa0JXb3ZzeXI5R1RIcXcz?=
 =?utf-8?B?Q2pLcExNdENBYlg4MHA1NjhYeUNOZkhWYkg3akEwSzl6em11RHVpS05SOGxr?=
 =?utf-8?B?UjVsUFVMOTN1MFBKRTZWQURtaGFoWFZKU3duLzlSOFNBSXlvY05qVXM4Yysy?=
 =?utf-8?B?ejZRdFVwTVo2c1A0L0dTaVpBVXFqYkgzU2tiNHovUlF5eWcxYWJCcDFIUjh2?=
 =?utf-8?B?MEhNajNGOXJuYVBEUmNlQkE1TUNHUi8yTzdjb01XVStkcUJHbm00Q0Ewb3NH?=
 =?utf-8?B?TGJDNkhUZFhOaDR2ZjkzSHB0b1BUMktSa3cxdmx5dm0vMTc0YTJSVzBrR3BV?=
 =?utf-8?B?bFduNEJtWWhUdW1ObExlSnJUVjZzOVdWaHpsVDRFdjZvbDRzTVV1bmVCbENi?=
 =?utf-8?B?QzdLYU9tc015TmQreWdkeEVzekZiSUh0MFVseFdEaTNYWnFIcmxxYkZlRUFC?=
 =?utf-8?B?bE1ZVVo4dVVhNW8xMEMwa3AwMTNTZC9qWFV0MS83dWl5T0pVa292UHlNRkEr?=
 =?utf-8?B?Q3hRT3RLM09xaCtyTnROaUx3VEJRS3JiUURvRnJsTjFtOUdsS2czWmNOSUlO?=
 =?utf-8?B?NVgvMHZyM0NUQUpRNjhrVTl1Z1NIa2dVNjR4VWJFcGRJK20wYXE4MXYxMGhy?=
 =?utf-8?B?bFE9PQ==?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 88fe95c1-fc3f-4736-d397-08db03ad7af4
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB5762.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2023 17:06:23.1554
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: buO4+umy9KhHkTQdKaAXQlCm/9Bn22JvONhK/1uTYtESet6jf6HfPiYDKbdqalIYOa+ZGIbQwcxrAvEBabRRvQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR12MB7123
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/31/23 10:17, Shanker Donthineni wrote:
> 
> 
> On 1/31/23 02:59, Thomas Gleixner wrote:
>> On Sun, Jan 29 2023 at 18:57, Shanker Donthineni wrote:
>>> +/* hlist_head to handle software resend of interrupts: */
>>> +static HLIST_HEAD(irq_resend_list);
>>> +static DEFINE_RAW_SPINLOCK(irq_resend_lock);
>>>
>>>   /*
>>>    * Run software resends of IRQ's
>>> @@ -30,18 +31,16 @@ static DECLARE_BITMAP(irqs_resend, IRQ_BITMAP_BITS);
>>>   static void resend_irqs(struct tasklet_struct *unused)
>>>   {
>>>        struct irq_desc *desc;
>>> -     int irq;
>>> -
>>> -     while (!bitmap_empty(irqs_resend, nr_irqs)) {
>>> -             irq = find_first_bit(irqs_resend, nr_irqs);
>>> -             clear_bit(irq, irqs_resend);
>>> -             desc = irq_to_desc(irq);
>>> -             if (!desc)
>>> -                     continue;
>>> +     struct hlist_node *n;
>>> +
>>> +     raw_spin_lock_irq(&irq_resend_lock);
>>> +     hlist_for_each_entry_safe(desc, n, &irq_resend_list, resend_node) {
>>> +             hlist_del_init(&desc->resend_node);
>>>                local_irq_disable();
>>>                desc->handle_irq(desc);
>>>                local_irq_enable();
>>>        }
>>> +     raw_spin_unlock_irq(&irq_resend_lock);
>> The lock ordering is broken here. irq_sw_resend() is invoked with
>> desc->lock held and takes irq_resend_lock.
>>
>> Lockdep clearly would have told you...
>>
>>          raw_spin_lock_irq(&irq_resend_lock);
>>          while (!hlist_empty(...)) {
>>                  desc = hlist_entry(irq_resend_list.first, ...);
>>                  hlist_del_init(&desc->resend_node);
>>                  raw_spin_unlock(&...);
>>                  desc->handle_irq();
>>                  raw_spin_lock(&...);
>>          }
> 
> Thanks for catching mistakes, I'll change it to this, please correct me if
> I'm doing another mistake.
> 
> static void resend_irqs(struct tasklet_struct *unused)
> {
>          struct irq_desc *desc;
> 
>          raw_spin_lock_irq(&irq_resend_lock);
>          while (hlist_empty(&irq_resend_list)) {

Sorry typo "while (!hlist_empty(&irq_resend_list)) {"


>                  desc = hlist_entry(irq_resend_list.first, struct irq_desc,
>                                     resend_node);
>                  hlist_del_init(&desc->resend_node);
>                  raw_spin_unlock(&irq_resend_lock);
>                  desc->handle_irq(desc);
>                  raw_spin_lock(&irq_resend_lock);
>          }
>          raw_spin_unlock_irq(&irq_resend_lock);
> }
> 
> Thanks,
> Shanker