LKML Archive on lore.kernel.org
 help / color / Atom feed
From: John Garry <john.garry@huawei.com>
To: "Ahmed S. Darwish" <a.darwish@linutronix.de>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Jason Yan <yanaijie@huawei.com>, Daniel Wagner <dwagner@suse.de>,
	Artur Paszkiewicz <artur.paszkiewicz@intel.com>,
	Jack Wang <jinpu.wang@cloud.ionos.com>,
	<linux-scsi@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Sebastian A. Siewior" <bigeasy@linutronix.de>
Subject: Re: [PATCH v2 00/19] scsi: libsas: Remove in_interrupt() check
Date: Tue, 12 Jan 2021 16:00:57 +0000
Message-ID: <e9bc0c89-a4d6-1e5b-793d-3c246882210e@huawei.com> (raw)
In-Reply-To: <X/2h0yNqtmgoLIb+@lx-t490>

- intel-linux-scu@intel.com

On 12/01/2021 13:19, Ahmed S. Darwish wrote:
> On Tue, Jan 12, 2021 at 11:53:50AM +0000, John Garry wrote:
>> On 12/01/2021 11:06, Ahmed S. Darwish wrote:
>>> Hi,
>>>
>>> Changelog v2
>>> ------------
> ...
>>
>> I'll give this a spin today and help review also then.
>>

I boot-tested on my machines which have hisi_sas v2 and v3 hw, and it's 
ok. I will ask some guys to test a bit more.

And generally the changes look ok. But I just have a slight concern that 
we don't pass the gfp_flags all the way from the origin caller.

So we have some really long callchains, for example:

host.c: sci_controller_error_handler(): atomic, irq handler     (*)
OR host.c: sci_controller_completion_handler(), atomic, tasklet (*)
   -> sci_controller_process_completions()
     -> sci_controller_unsolicited_frame()
       -> phy.c: sci_phy_frame_handler()
         -> sci_change_state(SCI_PHY_SUB_AWAIT_SAS_POWER)
           -> sci_phy_starting_await_sas_power_substate_enter()
             -> host.c: sci_controller_power_control_queue_insert()
               -> phy.c: sci_phy_consume_power_handler()
                 -> sci_change_state(SCI_PHY_SUB_FINAL)
         -> sci_change_state(SCI_PHY_SUB_FINAL)
     -> sci_controller_event_completion()
       -> phy.c: sci_phy_event_handler()
         -> sci_phy_start_sata_link_training()
           -> sci_change_state(SCI_PHY_SUB_AWAIT_SATA_POWER)
             -> sci_phy_starting_await_sata_power_substate_enter
               -> host.c: sci_controller_power_control_queue_insert()
                 -> phy.c: sci_phy_consume_power_handler()
                   -> sci_change_state(SCI_PHY_SUB_FINAL)

So if someone rearranges the code later, adds new callchains, etc., it 
could be missed that the context may have changed than what we assume at 
the bottom. But then passing the flags everywhere is cumbersome, and all 
the libsas users see little or no significant changes anyway, apart from 
a couple.

Thanks,
John


  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-12 11:06 Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 01/19] Documentation: scsi: libsas: Remove notify_ha_event() Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 02/19] scsi: libsas and users: Remove notifier indirection Ahmed S. Darwish
2021-01-12 11:36   ` John Garry
2021-01-12 12:09     ` Ahmed S. Darwish
2021-01-12 13:07     ` [PATCH v3 " Ahmed S. Darwish
2021-01-12 15:43       ` Christoph Hellwig
2021-01-12 11:06 ` [PATCH v2 03/19] scsi: libsas: Introduce a _gfp() variant of event notifiers Ahmed S. Darwish
2021-01-12 15:45   ` Christoph Hellwig
2021-01-12 17:12     ` Sebastian A. Siewior
2021-01-12 11:06 ` [PATCH v2 04/19] scsi: mvsas: Pass gfp_t flags to libsas " Ahmed S. Darwish
2021-01-12 15:46   ` Christoph Hellwig
2021-01-12 17:03     ` Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 05/19] scsi: isci: port: link down: Pass gfp_t flags Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 06/19] scsi: isci: port: link up: " Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 07/19] scsi: isci: port: broadcast change: " Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 08/19] scsi: libsas: Pass gfp_t flags to event notifiers Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 09/19] scsi: pm80xx: Pass gfp_t flags to libsas " Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 10/19] scsi: aic94xx: " Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 11/19] scsi: hisi_sas: " Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 12/19] scsi: libsas: event notifiers API: Add gfp_t flags parameter Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 13/19] scsi: hisi_sas: Switch back to original libsas event notifiers Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 14/19] scsi: aic94xx: " Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 15/19] scsi: pm80xx: " Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 16/19] scsi: libsas: Switch back to original event notifiers API Ahmed S. Darwish
2021-01-12 14:42   ` John Garry
2021-01-12 11:06 ` [PATCH v2 17/19] scsi: isci: Switch back to original libsas event notifiers Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 18/19] scsi: mvsas: " Ahmed S. Darwish
2021-01-12 11:06 ` [PATCH v2 19/19] scsi: libsas: Remove temporarily-added _gfp() API variants Ahmed S. Darwish
2021-01-12 11:53 ` [PATCH v2 00/19] scsi: libsas: Remove in_interrupt() check John Garry
2021-01-12 13:19   ` Ahmed S. Darwish
2021-01-12 16:00     ` John Garry [this message]
2021-01-12 17:33       ` Ahmed S. Darwish
2021-01-14  9:51         ` John Garry
2021-01-15 16:27           ` Ahmed S. Darwish
2021-01-15 16:29             ` John Garry
2021-01-15 16:41               ` Ahmed S. Darwish

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e9bc0c89-a4d6-1e5b-793d-3c246882210e@huawei.com \
    --to=john.garry@huawei.com \
    --cc=a.darwish@linutronix.de \
    --cc=artur.paszkiewicz@intel.com \
    --cc=bigeasy@linutronix.de \
    --cc=dwagner@suse.de \
    --cc=jejb@linux.ibm.com \
    --cc=jinpu.wang@cloud.ionos.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=tglx@linutronix.de \
    --cc=yanaijie@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lkml.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lkml.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lkml.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lkml.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lkml.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lkml.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lkml.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lkml.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lkml.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lkml.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lkml.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lkml.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git