Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 30/05/2018 15:28, Sebastian Andrzej Siewior wrote:
On 2018-05-30 15:08:37 [+0100], John Garry wrote:
On 30/05/2018 14:45, Sebastian Andrzej Siewior wrote:
excellent. So no more objections from your side or is this a complaint I
didn't fully decode?

I think the original code is not great since we're dropping the lock but
maintaining the irq posture as disabled.

the patch does not change the locking as it exists today.

Right


Do you plan to add a lockdep_assert_irqs_disabled() check in addition? It's
not needed if we work on the basis that the lock is always taken with irqs
disabled. And is this lockdep function even intended to be used outside core
kernel code?

I don't plan to add lockdep_assert_irqs_disabled(). You should only need
to use lockdep_assert_held() to verify that the lock is held as
expected. Whether or not the interrupts need to be disabled is best
verified with lockdep. Lockdep would even complain here if you intend to
invoke spin_unlock() on a lock which was not acquired earlier (I'm just
trying to say that lockdep_assert_held() is not required here either).

Personally I think that solving the issue in the original code would be
better, i.e. keeping irqs disabled needlessly. Or else the "TODO" can stay
(or be amended to improve clarity).

I would like to get rid of that local_irq_save() here because it breaks
-RT and is not the correct thing to do for !RT (but it does not break
things).
I posted v1 with just the removal of the local_irq_sav() and v2 with
TODO removed as suggested here in the thread. The interrupts still
remain disabled.
So based on that it seems that you are in favour of the initial v1.


In fact, if this functional change was accepted then the "TODO" would need to be updated for clarity since it would be less clear in meaning when the local_irq_save() function is removed.

However, since !RT is not broken, then I am not sure if we accept changes to fix the RT kernel only. You will need to defer to the James and Martin for that.

Thanks,
John

John

Sebastian

.






[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux