Looks good,
Reviewed-by: Johannes Thumshirn
--
Johannes Thumshirn Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG
Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
management duties from lldds") the sas_ata_qc_issue() function unlocks
the ata_port.lock and disables interrupts before doing so.
That lock is always taken with disabled interrupts so at this point, the
interrupts are already
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
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
>
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();
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
On 30/05/2018 12:22, Sebastian Andrzej Siewior wrote:
On 2018-05-30 12:16:23 [+0100], John Garry wrote:
On 30/05/2018 11:16, Sebastian Andrzej Siewior wrote:
On 2018-05-30 10:34:12 [+0100], John Garry wrote:
Sorry, but personally I don't see much value in this change. I think it's
better for
On 2018-05-30 12:16:23 [+0100], John Garry wrote:
> On 30/05/2018 11:16, Sebastian Andrzej Siewior wrote:
> > On 2018-05-30 10:34:12 [+0100], John Garry wrote:
> > > Sorry, but personally I don't see much value in this change. I think it's
> > > better for safety to be consistent in how we lock &
On 30/05/2018 11:16, Sebastian Andrzej Siewior wrote:
On 2018-05-30 10:34:12 [+0100], John Garry wrote:
Sorry, but personally I don't see much value in this change. I think it's
better for safety to be consistent in how we lock & unlock the spinlock,
i.e. use irqsave variant (or similar).
The
On 2018-05-30 10:34:12 [+0100], John Garry wrote:
> Sorry, but personally I don't see much value in this change. I think it's
> better for safety to be consistent in how we lock & unlock the spinlock,
> i.e. use irqsave variant (or similar).
The lock should do irqsave() and unlock irqrestore().
On 23/05/2018 10:24, Sebastian Andrzej Siewior wrote:
On 2018-05-23 09:46:30 [+0100], John Garry wrote:
On 22/05/2018 18:31, Sebastian Andrzej Siewior wrote:
On 2018-05-18 14:31:27 [+0100], John Garry wrote:
On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
Since commit 312d3e56119a
On 2018-05-24 16:20:33 [+0800], Jason Yan wrote:
> NO, this:
>
> /* TODO: audit callers to ensure they are ready for qc_issue to
>* unconditionally re-enable interrupts
>*/
> local_irq_save(flags);
> spin_unlock(ap->lock);
indeed, I have no idea how I could have
On 2018/5/24 16:18, Sebastian Andrzej Siewior wrote:
On 2018-05-24 16:00:47 [+0800], Jason Yan wrote:
Because you are trying to delete the irq-save code which will make the
comment hard to understand, I'm just giving an advise. It's welcome
if you have a better way.
I am removing
On 2018-05-24 16:00:47 [+0800], Jason Yan wrote:
> > > Because you are trying to delete the irq-save code which will make the
> > > comment hard to understand, I'm just giving an advise. It's welcome
> > > if you have a better way.
> >
> > I am removing local_irq_save() prior an unlock of a lock
On 2018/5/24 15:52, Sebastian Andrzej Siewior wrote:
On 2018-05-24 15:38:51 [+0800], Jason Yan wrote:
no, please don't do this. Please add instead
lockdep_assert_held()
on the lock in question and let lockdep to its work. Lockdep has way
better coverage than your irqs_disabled()
On 2018/5/24 15:50, Johannes Thumshirn wrote:
On Thu, May 24, 2018 at 03:38:51PM +0800, Jason Yan wrote:
On 2018/5/24 15:15, Sebastian Andrzej Siewior wrote:
On 2018-05-24 10:58:44 [+0800], Jason Yan wrote:
I think it's fine to delete this irq save code. As for the "TODO"
comment, I think
On 2018-05-24 09:50:07 [+0200], Johannes Thumshirn wrote:
> > lockdep_assert_held() cannot detect the irq state, it can only detect
> > whether we have held the lock.
>
> I think Sebastian wanted to say lockdep_assert_irqs_disabled().
nope, meant the right thing.
> Byte,
> Johannes
On 2018-05-24 15:38:51 [+0800], Jason Yan wrote:
> > no, please don't do this. Please add instead
> > lockdep_assert_held()
> >
> > on the lock in question and let lockdep to its work. Lockdep has way
> > better coverage than your irqs_disabled() check which also breaks RT.
> >
>
>
On Thu, May 24, 2018 at 03:38:51PM +0800, Jason Yan wrote:
>
>
> On 2018/5/24 15:15, Sebastian Andrzej Siewior wrote:
> > On 2018-05-24 10:58:44 [+0800], Jason Yan wrote:
> > > I think it's fine to delete this irq save code. As for the "TODO"
> > > comment, I think we can add:
> > >
> > >
On 2018/5/24 15:15, Sebastian Andrzej Siewior wrote:
On 2018-05-24 10:58:44 [+0800], Jason Yan wrote:
I think it's fine to delete this irq save code. As for the "TODO"
comment, I think we can add:
BUG_ON(!irqs_disabled());
or
WARN_ON_ONCE(!irqs_disabled());
no, please don't do this.
On 2018-05-24 10:58:44 [+0800], Jason Yan wrote:
> I think it's fine to delete this irq save code. As for the "TODO"
> comment, I think we can add:
>
> BUG_ON(!irqs_disabled());
>
> or
>
> WARN_ON_ONCE(!irqs_disabled());
no, please don't do this. Please add instead
On 2018/5/23 17:24, Sebastian Andrzej Siewior wrote:
On 2018-05-23 09:46:30 [+0100], John Garry wrote:
On 22/05/2018 18:31, Sebastian Andrzej Siewior wrote:
On 2018-05-18 14:31:27 [+0100], John Garry wrote:
On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
Since commit 312d3e56119a
On 2018-05-23 09:46:30 [+0100], John Garry wrote:
> On 22/05/2018 18:31, Sebastian Andrzej Siewior wrote:
> > On 2018-05-18 14:31:27 [+0100], John Garry wrote:
> > > On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
> > > > Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
> > >
On 22/05/2018 18:31, Sebastian Andrzej Siewior wrote:
On 2018-05-18 14:31:27 [+0100], John Garry wrote:
On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
management duties from lldds") the sas_ata_qc_issue() function unlocks
On 2018-05-18 14:31:27 [+0100], John Garry wrote:
> On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
> > Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
> > management duties from lldds") the sas_ata_qc_issue() function unlocks
> > the ata_port.lock and disables interrupts
On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
management duties from lldds") the sas_ata_qc_issue() function unlocks
the ata_port.lock and disables interrupts before doing so.
That lock is always taken with disabled
Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
management duties from lldds") the sas_ata_qc_issue() function unlocks
the ata_port.lock and disables interrupts before doing so.
That lock is always taken with disabled interrupts so at this point, the
interrupts are already
27 matches
Mail list logo
