Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
From:   Niklas Cassel <Niklas.Cassel@wdc.com>
To:     Sasha Levin <sashal@kernel.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        Damien Le Moal <dlemoal@kernel.org>,
        "linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>
Subject: Re: [PATCH AUTOSEL 4.14 6/6] ata: libata-eh: do not clear
 ATA_PFLAG_EH_PENDING in ata_eh_reset()
Thread-Topic: [PATCH AUTOSEL 4.14 6/6] ata: libata-eh: do not clear
 ATA_PFLAG_EH_PENDING in ata_eh_reset()
Thread-Index: AQHZ7un7gzcVNpTh1Ea2XyoS5/QAzbAwa+2A
Date:   Thu, 28 Sep 2023 16:00:24 +0000
Message-ID: <ZRWjE7e8EQgL41ae@x1-carbon>
References: <20230924132050.1277021-1-sashal@kernel.org>
 <20230924132050.1277021-6-sashal@kernel.org>
In-Reply-To: <20230924132050.1277021-6-sashal@kernel.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?XSj8ob7eJgxkmoDDmdsVf21tJUjQdzlr9G7fRlli3S6gXcue+4S2ytfaAxmE?=
 =?us-ascii?Q?/nJNs62AXFoiI8WtVGw71+KqkazFTwHgw3hZp16gzVjEcWjOim9xLEhaJmDF?=
 =?us-ascii?Q?IM/gx3kHGLzupWjifpmjYjoteA5aLOa9744S9RFlV++KSRfscRHOhgtg6HPb?=
 =?us-ascii?Q?e9SOBz8XXbfav18pFKjqrokNbtAfFpKP1pg1rCMEnu6uHTOoGORZzbiqXn0p?=
 =?us-ascii?Q?RER8tN1JFUAONIkQHClj7s7QGb5Nbw5KV/tfxaBG9exQIxEk6Yv/ud7sd0H0?=
 =?us-ascii?Q?8wkoyKKH52Rkxj2VHLfNkhcDfe/3HaAs/htZL10wpckNy7vQyNv7xPHYGOAj?=
 =?us-ascii?Q?ksz9spRYlI11/txMOdQkXqWsbXI08wUIBAjT/h8dZCaQJC+Bq2os8Ckh36ic?=
 =?us-ascii?Q?ZG8qVdztRoqG6QER8DNITlheWQGL+vzH/Kbmh3IP+5n1aamp8c6gOKV330VB?=
 =?us-ascii?Q?02iDU8pcOeIcEtLvYW4bGIe7jamGvRHh3ZxlFU40JiYLEqEYjVEFqkIUDoNe?=
 =?us-ascii?Q?+XHQKcrZBDBjC0AVOsb7EzQaEp89Gn7iX82ec8z9Hj/jgfCsxw2BlQvGpGou?=
 =?us-ascii?Q?ceiq4L89Qiv2uie9X98XdWTvR0R3almAxHWDbHzSHiiV75ODPIHv9P9BrBDY?=
 =?us-ascii?Q?lVSBEG3d7sb3n1w2eic6zmeAoArkpHYqab1dqYvxfJcpgXXqu6wejTdiopBK?=
 =?us-ascii?Q?jgHBDVXTHtMUP3/VT+5eXlQgn56xqNF3vx+pY+3l1OCq44Cd/f37KNkUbGtb?=
 =?us-ascii?Q?ymDgq5ArCdfxC8jZO6+3FcynDVENABU9+yC0bVFhrdqcCnRQ2IIqczr/sygO?=
 =?us-ascii?Q?MHEDxxWqpO+Ng8NToYMQKMPoqMUUD+vEhbbaAYmwf/9fb7K8ViTYjWyISyLK?=
 =?us-ascii?Q?sfqSi/uvxII49pbQqqlLUiRITrdUnx0hMSgEKJZRHBg+Nt9ERGANEb5W6IDP?=
 =?us-ascii?Q?9V6axhlAKnVUqUCtm2e6XzQ2S97OsNurHoCG308xYs3Mp56cHoIGQ0Qipjup?=
 =?us-ascii?Q?EDJL3FB0mwVIJyKV+agmsMdAgdD9b2kZlPUFMK/quF0aEVRxTc2S6549bWOt?=
 =?us-ascii?Q?2yQHRV1DaATN7JsD4kDad/RbCTrw7Sar/fXQOE68fK8szbEq39BirbpBiZqW?=
 =?us-ascii?Q?b/XFYd5+e1KxtDisp3ol0KTHAQ3mBN9Iyf+50RD7LmD0UOKQR+VKzXMZaXRH?=
 =?us-ascii?Q?9pXgwgAxQg3faNkO5aoXw/XDvnUt2YGm9S8d63oodvEZ0OF0wxHeF0ZsbxwY?=
 =?us-ascii?Q?9c5Jp0La2MUfmnyX9KA7qVkZd8NKXc8aC9HW97H+PGyR3XSEOeyJJD6lUOZq?=
 =?us-ascii?Q?TV/uWXSIDxEHTRlsp/33oTMaV5v0GFl6kgIMnn7LL2lKhz3ehR03fCT8r+fO?=
 =?us-ascii?Q?tPyQTjaxk+OY7gkhUQNwno6lXoRbortJ7kyuBi0HjdCR2hS9p1RLHzmpVZbk?=
 =?us-ascii?Q?YTVOCkZl9/ZDh1LMlDsEs4HcF4ZN/XAYrROMcw6IllUlvRU1q/N8dsKl52jh?=
 =?us-ascii?Q?ztVO4kncGtmLAi4s9qjpATndJ6PA9j5jfYHZy2TPF5IDOR1tYQiry9yx7e5U?=
 =?us-ascii?Q?rVm8lyBqvjcDQeBMJrBUOOCU4bKoWLyw1//9Eiav/oWd2mnLar74saqTK4A/?=
 =?us-ascii?Q?lg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <438869F821C27946BC6AD77A7E8B3DEA@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: MQBKFMZh7gv3OZyRVeHfwC2Brpak1Yfa8blgmg0W1q9rClfbpwy+Qs8qiwwyg1WTwB4yec04c535LmpkcnS0w2sHKoNcAe2sDpJ2+mzE/ZUVbcJrksxVT5IrUTZPraouqKLKkAMz/yW1uLLct8E5DrW7wuDBiM64BQPGWRpTkO3ZVVsfkxU5NcqrL0Lpw1Nau/8xEFVGftJxXw0JcFgTvEWrotP9UnFHVObnY4sdzaF37YSKNq4AnXocQ3z0q/gqpqthGe2Nkg0ZFFfY6WvOop4sShhdPp3Z/B390KV/OmLSPCNSTPBzp6L2J9PN7+uU+sjCohfeSCMjjFKtpxbnBw3qMi8nQFGRB0utsNpOwl/W8QORYK/P8h9OE1dmEv4UXTvP9dnofASUZlQvJTaSSF4JgsAYVw77i6SaxEUhXo+kJv6P7kvkncx+5aIxtQKJ2oTDkehi+mWhBCUvrV3vMWs59DrJYa0mBPVn+XLCtYJtvhGaTTM/F85fVIYYQ2s4D6zW6UkwLP/NOrPM0xqHPCMZSk34dirJeYzYws3m06shm5EZ+thRbzJd7Mu9CeZdLJL3Twe+fwvH9rFxdhdV4ShFNFk5m0X42GmnUUvrJj1oL81MpIVcwxrOd6KfOESpqw7VKT3ffaAzK8BLsRUNLodRC15uVnz8ATlMbq8RBVeDI8TMmiUdbBHow3MJAwFejwFAmIKwt1FbPJ9G0kT60NS3R22562q7p+cgfL4xi/+vWN0zXd+RwqXidJCylm+zIjCP7ViKnG/gtmQFfUd519QQzPrHecm//EchOEYf7BgJINTJ9O0+5S2YPE+Fi8zt
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MN2PR04MB6272.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8b607224-d659-4093-a636-08dbc03c0699
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Sep 2023 16:00:24.3876
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 2UGk+N38bii/9hWC5FFjwYhYwYV22PZUSw+KAtdPMUjDvvfNKafdM/+Pz1eZgKvYP42s6uH6cgrbsO771xq/FQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR04MB6309
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,
	SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Thu, 28 Sep 2023 09:00:47 -0700 (PDT)

Hello Sasha,

The upstream commit that you are backporting here:
80cc944eca4f ("ata: libata-eh: do not clear ATA_PFLAG_EH_PENDING in ata_eh_=
reset()")

Requires that we have:
737dd811a3db ("ata: libahci: clear pending interrupt status")
in stable. This is currently not the case.

See my comment in:
https://lore.kernel.org/stable/ZRWf7Avtdv3DeqCU@x1-carbon/T/#t

Perhaps both:
93c7711494f4 ("ata: ahci: Drop pointless VPRINTK() calls and convert the re=
maining ones")
and
737dd811a3db ("ata: libahci: clear pending interrupt status")

Could be backported to stable.
That way,
80cc944eca4f ("ata: libata-eh: do not clear ATA_PFLAG_EH_PENDING in ata_eh_=
reset()")
should be safe to backport to stable as well.


Kind regards,
Niklas


On Sun, Sep 24, 2023 at 09:20:49AM -0400, Sasha Levin wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
>=20
> [ Upstream commit 80cc944eca4f0baa9c381d0706f3160e491437f2 ]
>=20
> ata_scsi_port_error_handler() starts off by clearing ATA_PFLAG_EH_PENDING=
,
> before calling ap->ops->error_handler() (without holding the ap->lock).
>=20
> If an error IRQ is received while ap->ops->error_handler() is running,
> the irq handler will set ATA_PFLAG_EH_PENDING.
>=20
> Once ap->ops->error_handler() returns, ata_scsi_port_error_handler()
> checks if ATA_PFLAG_EH_PENDING is set, and if it is, another iteration
> of ATA EH is performed.
>=20
> The problem is that ATA_PFLAG_EH_PENDING is not only cleared by
> ata_scsi_port_error_handler(), it is also cleared by ata_eh_reset().
>=20
> ata_eh_reset() is called by ap->ops->error_handler(). This additional
> clearing done by ata_eh_reset() breaks the whole retry logic in
> ata_scsi_port_error_handler(). Thus, if an error IRQ is received while
> ap->ops->error_handler() is running, the port will currently remain
> frozen and will never get re-enabled.
>=20
> The additional clearing in ata_eh_reset() was introduced in commit
> 1e641060c4b5 ("libata: clear eh_info on reset completion").
>=20
> Looking at the original error report:
> https://marc.info/?l=3Dlinux-ide&m=3D124765325828495&w=3D2
>=20
> We can see the following happening:
> [    1.074659] ata3: XXX port freeze
> [    1.074700] ata3: XXX hardresetting link, stopping engine
> [    1.074746] ata3: XXX flipping SControl
>=20
> [    1.411471] ata3: XXX irq_stat=3D400040 CONN|PHY
> [    1.411475] ata3: XXX port freeze
>=20
> [    1.420049] ata3: XXX starting engine
> [    1.420096] ata3: XXX rc=3D0, class=3D1
> [    1.420142] ata3: XXX clearing IRQs for thawing
> [    1.420188] ata3: XXX port thawed
> [    1.420234] ata3: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>=20
> We are not supposed to be able to receive an error IRQ while the port is
> frozen (PxIE is set to 0, i.e. all IRQs for the port are disabled).
>=20
> AHCI 1.3.1 section 10.7.1.1 First Tier (IS Register) states:
> "Each bit location can be thought of as reporting a '1' if the virtual
> "interrupt line" for that port is indicating it wishes to generate an
> interrupt. That is, if a port has one or more interrupt status bit set,
> and the enables for those status bits are set, then this bit shall be set=
."
>=20
> Additionally, AHCI state P:ComInit clearly shows that the state machine
> will only jump to P:ComInitSetIS (which sets IS.IPS(x) to '1'), if PxIE.P=
CE
> is set to '1'. In our case, PxIE is set to 0, so IS.IPS(x) won't get set.
>=20
> So IS.IPS(x) only gets set if PxIS and PxIE is set.
>=20
> AHCI 1.3.1 section 10.7.1.1 First Tier (IS Register) also states:
> "The bits in this register are read/write clear. It is set by the level o=
f
> the virtual interrupt line being a set, and cleared by a write of '1' fro=
m
> the software."
>=20
> So if IS.IPS(x) is set, you need to explicitly clear it by writing a 1 to
> IS.IPS(x) for that port.
>=20
> Since PxIE is cleared, the only way to get an interrupt while the port is
> frozen, is if IS.IPS(x) is set, and the only way IS.IPS(x) can be set whe=
n
> the port is frozen, is if it was set before the port was frozen.
>=20
> However, since commit 737dd811a3db ("ata: libahci: clear pending interrup=
t
> status"), we clear both PxIS and IS.IPS(x) after freezing the port, but
> before the COMRESET, so the problem that commit 1e641060c4b5 ("libata:
> clear eh_info on reset completion") fixed can no longer happen.
>=20
> Thus, revert commit 1e641060c4b5 ("libata: clear eh_info on reset
> completion"), so that the retry logic in ata_scsi_port_error_handler()
> works once again. (The retry logic is still needed, since we can still
> get an error IRQ _after_ the port has been thawed, but before
> ata_scsi_port_error_handler() takes the ap->lock in order to check
> if ATA_PFLAG_EH_PENDING is set.)
>=20
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/ata/libata-eh.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>=20
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index cbe9af624a06f..8a789de056807 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2948,18 +2948,11 @@ int ata_eh_reset(struct ata_link *link, int class=
ify,
>  			postreset(slave, classes);
>  	}
> =20
> -	/*
> -	 * Some controllers can't be frozen very well and may set spurious
> -	 * error conditions during reset.  Clear accumulated error
> -	 * information and re-thaw the port if frozen.  As reset is the
> -	 * final recovery action and we cross check link onlineness against
> -	 * device classification later, no hotplug event is lost by this.
> -	 */
> +	/* clear cached SError */
>  	spin_lock_irqsave(link->ap->lock, flags);
> -	memset(&link->eh_info, 0, sizeof(link->eh_info));
> +	link->eh_info.serror =3D 0;
>  	if (slave)
> -		memset(&slave->eh_info, 0, sizeof(link->eh_info));
> -	ap->pflags &=3D ~ATA_PFLAG_EH_PENDING;
> +		slave->eh_info.serror =3D 0;
>  	spin_unlock_irqrestore(link->ap->lock, flags);
> =20
>  	if (ap->pflags & ATA_PFLAG_FROZEN)
> --=20
> 2.40.1
> =