Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 5 Jan 2023 15:24:58 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
CC:     Adaptec OEM Raid Solutions <aacraid@microsemi.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        <linux-scsi@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] scsi: ips: Replace kmap_atomic() with kmap_local_page()
Message-ID: <Y7dcSswZppY4hn3H@iweiny-desk3>
References: <20230103173131.21259-1-fmdefrancesco@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230103173131.21259-1-fmdefrancesco@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?EpTqEmeRL/VdsDB+MhRJgY8w9N2SlfkShxL9OvgI9sK9eFLe/jEtRSa+y4bq?=
 =?us-ascii?Q?J0IRtOi4TKbPGTpD0Q2zB6DFF1fmqywkRXr8lRq5Y+7MtgSgkgc+BMo8xGHf?=
 =?us-ascii?Q?09Q9AHZI6eE2u6K2xAW6tQJutVr42qCPbdOw72RI/gMIVeW2TrjNuGvvI5Cs?=
 =?us-ascii?Q?VN6QCGTCEXUVBch0Qir7HoFPXaIVT/Ch4fqOJpxPrC3zmxpNah0DZ2t9Cb4p?=
 =?us-ascii?Q?BhJAzvEb6dKNXu9vdFGnf+j7KyDpx1LnAYp0k7UzLiHYfVTkWje3Qr4V4Ev2?=
 =?us-ascii?Q?SLZOL05r0eHrynyTgnzh6CC6Kr2ujwzqOYiopTev9Uvjr4dV78X16t6PVvmn?=
 =?us-ascii?Q?7cw+v4jewaEVBRLt16/vjmmqj9oQmKtQxZofr4rbYaz5Mm9KL4ztB11xijPW?=
 =?us-ascii?Q?YXxJEgRk5ktBw8EYT/HcNuQ31B8N1ZZduT8vbDJDlF9j4Xriyg3crEjx2z92?=
 =?us-ascii?Q?NrrAmgSR+gizjqwXeslphGFKOuvqvkyeggaHeJNHVrv6aqyBiSivwm2/9rJi?=
 =?us-ascii?Q?ZkNcB5zEgRucNKGnZ8GAjwAz8jWCH8yeod79djD1sLkhXk38Z2e64F2zY/Vg?=
 =?us-ascii?Q?dUWCh+hIFfyaWp/r9aK0dpir2g/10sgKmeSsDmSH99/BoHFSECU2FvWYlVRS?=
 =?us-ascii?Q?/sCLaxLrb7bHehv1ZaM8n2+tWN5+yv6p0WqWC2EhndG2I7ANjZyFAytIVGrd?=
 =?us-ascii?Q?qvqkMKNDzQLS05bcHwVWVfS9X6ZgqbQZy3e0OrmZAOxykIGczGBiJ5n1ARsN?=
 =?us-ascii?Q?ZkqxzBBZrPV3X9LD9VLB1LQ1Yu52Iqi1Cg6WT9atCvgzNHRbBRfS3gjEA/5s?=
 =?us-ascii?Q?w1EyEK2jOFoD2HwlPnemTzi/gsJ9QHVS8HYVYyeUCXAFQpXBj9DOpPiqONeO?=
 =?us-ascii?Q?B/u/ZfD9gI7J0rISX5zfL0WEKNVFtUIfiAOWJ3bZc0NHIk5th3svODIn981J?=
 =?us-ascii?Q?7bJUbED4NQW4hFv7dWdxaRCaUUocslH9QcV3vyoVpxWuRSufDJeQVUbu1o/e?=
 =?us-ascii?Q?L7ymTvsTr7qoqow6zGOajhhkmsXK2YwsD4gbEdsg33mV8hPxXJ8WkSH2NiI6?=
 =?us-ascii?Q?4YGFbda5JF1xvl4268FX6c7GoUidhYrNqg8fRCiOvuWBLfCxcx9LuRn2jSu9?=
 =?us-ascii?Q?gi5gDBKKdMxmhIOj4eQK7xWSkTQOE5j/Je7cY+b29iSAWbTeDAPZfkEQUEfh?=
 =?us-ascii?Q?TxYvGaxHc8urHf/m6JZhUvLfF2Py7tgibjWj2yrUr0q5pHocwuBvIKV84NYT?=
 =?us-ascii?Q?u7RD70/Lwg4oyLcc1nRc5d2Jak4gBwqhSguye2wRbueNh90MpHU71T9J2fzS?=
 =?us-ascii?Q?1/6IQmq9yAojUzKUX4gk9c7/mMhHTqPvR+/MHesqSV+40JkHUS4ru0z8KGDU?=
 =?us-ascii?Q?g5UyqF048EuKPR1VFIkf4IRme/aSXXVrEMhsAXsaEMql8xwsNXWiuAmfXWiX?=
 =?us-ascii?Q?Pq5JIQNwgyA/Q7Bxcr48Yl9vVEntb3DBqypm0VX7FAztLspl5OLJaGAZLx9x?=
 =?us-ascii?Q?RLIoA2liYVQEPu9uKQQvsjE26PdSz7ERAykV1ZxQGQ53pLa3EkwqBkyNb4rV?=
 =?us-ascii?Q?XHM3HZyXWKK84VeARPrFdA5VONL2O70OOU78n5latkqjQ6yywwxRFRe8r9ej?=
 =?us-ascii?Q?GQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d44ea8b0-74fe-4ebc-82bc-08daef7411a1
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jan 2023 23:25:01.9732
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yQES8KeOVZpKNfcD+ZAHFQBykro+DACJVWBhKe5wd6WDn6IF3mBRNT9btCqjzrzmC4/k1G6xcXX2AgoGlO55ZQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB7375
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jan 03, 2023 at 06:31:31PM +0100, Fabio M. De Francesco wrote:
> kmap_atomic() is deprecated in favor of kmap_local_page(). Therefore,
> replace kmap_atomic() with kmap_local_page() in ips_is_passthru(). In
> the meantime remove an unnecessary comment soon before local mapping,
> align code and remove spaces (the function is short, therefore the
> reviewers job won't be over-complicated by these logically unrelated
> clean-ups).
> 
> kmap_atomic() is implemented like a kmap_local_page() which also disables
> page-faults and preemption (the latter only for !PREEMPT_RT kernels).
> The code within the mapping/unmapping in ips_is_passthru() is already
> in atomic context because of a call to local_irq_save() and
> kmap_local_page() can be called in atomic context too (including
> interrupts).
> 
> Therefore, a mere replacement of the old API with the new one is all it
> is required (i.e., there is no need to explicitly add any calls to
> pagefault_disable() and/or preempt_disable()).
> 
> Suggested-by: Ira Weiny <ira.weiny@intel.com>

The discussion about preemption is irrelevant AFAICS.  But what you say above
is not wrong and the code looks right.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/scsi/ips.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
> index 16419aeec02d..bb206509265e 100644
> --- a/drivers/scsi/ips.c
> +++ b/drivers/scsi/ips.c
> @@ -1499,17 +1499,16 @@ static int ips_is_passthru(struct scsi_cmnd *SC)
>                  struct scatterlist *sg = scsi_sglist(SC);
>                  char  *buffer;
>  
> -                /* kmap_atomic() ensures addressability of the user buffer.*/
>                  /* local_irq_save() protects the KM_IRQ0 address slot.     */
>                  local_irq_save(flags);
> -                buffer = kmap_atomic(sg_page(sg)) + sg->offset;
> -                if (buffer && buffer[0] == 'C' && buffer[1] == 'O' &&
> -                    buffer[2] == 'P' && buffer[3] == 'P') {
> -                        kunmap_atomic(buffer - sg->offset);
> +		buffer = kmap_local_page(sg_page(sg)) + sg->offset;
> +		if (buffer && buffer[0] == 'C' && buffer[1] == 'O' &&
> +		    buffer[2] == 'P' && buffer[3] == 'P') {
> +			kunmap_local(buffer);
>                          local_irq_restore(flags);
>                          return 1;
>                  }
> -                kunmap_atomic(buffer - sg->offset);
> +		kunmap_local(buffer);
>                  local_irq_restore(flags);
>  	}
>  	return 0;
> -- 
> 2.39.0
>