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 14:45, Sebastian Andrzej Siewior wrote:
On 2018-05-30 14:37:50 [+0100], John Garry wrote:
On 30/05/2018 12:22, Sebastian Andrzej Siewior wrote:
Yes, there are the same on vanilla and not on RT. However my point is
that the code does this instead:
	local_irq_save();
	spin_unlock();

Ah, I just noticed that this is spin_unlock().

So about the "TODO", which you mention "I *assumed* that the intention was
to audit the code for this

 	spin_unlock_irq(ap->lock);

change instead. But if this is or was never intended than I could indeed
remove the TODO comment."

As I see, we're dropping the lock but maintaining the irq posture for
holding that lock (disabled), which seems inefficient.

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.

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?

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).

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