Received-SPF: pass (google.com: domain of linux-kernel+bounces-186362-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 22 May 2024 17:24:40 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
Cc: =?iso-8859-1?Q?N=EDcolas_F=2E_R=2E_A=2E?= Prado <nfraprado@collabora.com>,
	Mark Brown <broonie@kernel.org>, linux-spi@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH v1 1/1] spi: Remove unneded check for orig_nents
Message-ID: <Zk4AKOoymq5OvlkA@smile.fi.intel.com>
References: <20240507201028.564630-1-andriy.shevchenko@linux.intel.com>
 <d8930bce-6db6-45f4-8f09-8a00fa48e607@notapiano>
 <8ae675b5-fcf9-4c9b-b06a-4462f70e1322@linaro.org>
 <Zk3X7Dgst5kVzJxy@smile.fi.intel.com>
 <5c32d7fd-4a7f-4d9c-805c-87d4d14f741e@linaro.org>
 <71e7b6f8-67f2-4c03-b83a-71d7e747ad04@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <71e7b6f8-67f2-4c03-b83a-71d7e747ad04@linaro.org>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Wed, May 22, 2024 at 03:18:18PM +0200, Neil Armstrong wrote:
> On 22/05/2024 13:53, Neil Armstrong wrote:
> > On 22/05/2024 13:33, Andy Shevchenko wrote:
> > > On Wed, May 22, 2024 at 12:03:33PM +0200, Neil Armstrong wrote:
> > > > On 15/05/2024 23:09, N?colas F. R. A. Prado wrote:
> > > > > On Tue, May 07, 2024 at 11:10:27PM +0300, Andy Shevchenko wrote:
> > > > > > Both dma_unmap_sgtable() and sg_free_table() in spi_unmap_buf_attrs()
> > > > > > have checks for orig_nents against 0. No need to duplicate this.
> > > > > > All the same applies to other DMA mapping API calls.
> > > > > > 
> > > > > > Also note, there is no other user in the kernel that does this kind of
> > > > > > checks.
> > > > > > 
> > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > this commit caused a regression which I reported here:
> > > > > 
> > > > > https://lore.kernel.org/all/d3679496-2e4e-4a7c-97ed-f193bd53af1d@notapiano
> > > > > 
> > > > > along with some thoughts on the cause and a possible solution, though I'm not
> > > > > familiar with this code base at all and would really appreciate any feedback you
> > > > > may have.
> > > > 
> > > > I also see the same regression on the SM8550 and SM8650 platforms,
> > > > please CC linux-arm-msm@vger.kernel.org and me for a potential fix to test on those platforms.
> > > 
> > > There is still no answer from IOMMU patch author. Do you have the same trace
> > > due to IOMMU calls? Anyway, I guess it would be nice to see it.
> > 
> > Yes :
> > [??? 6.404623] Unable to handle kernel NULL pointer dereference at virtual address 000000000000001c
> > <snip>
> > [??? 6.641597] lr : __dma_sync_sg_for_device+0x3c/0x40
> > <snip>
> > [??? 6.688286] Call trace:
> > [??? 6.688287]? iommu_dma_sync_sg_for_device+0x28/0x100
> > [??? 6.717582]? __dma_sync_sg_for_device+0x3c/0x40
> > [??? 6.717585]? spi_transfer_one_message+0x358/0x680
> > [??? 6.732229]? __spi_pump_transfer_message+0x188/0x494
> > [??? 6.732232]? __spi_sync+0x2a8/0x3c4
> > [??? 6.732234]? spi_sync+0x30/0x54
> > [??? 6.732236]? goodix_berlin_spi_write+0xf8/0x164 [goodix_berlin_spi]
> > [??? 6.739854]? _regmap_raw_write_impl+0x538/0x674
> > [??? 6.750053]? _regmap_raw_write+0xb4/0x144
> > [??? 6.750056]? regmap_raw_write+0x7c/0xc0
> > [??? 6.750058]? goodix_berlin_power_on+0xb0/0x1b0 [goodix_berlin_core]
> > [??? 6.765520]? goodix_berlin_probe+0xc0/0x660 [goodix_berlin_core]
> > [??? 6.765522]? goodix_berlin_spi_probe+0x12c/0x14c [goodix_berlin_spi]
> > [??? 6.772339]? spi_probe+0x84/0xe4
> > [??? 6.772342]? really_probe+0xbc/0x29c
> > [??? 6.784313]? __driver_probe_device+0x78/0x12c
> > [??? 6.784316]? driver_probe_device+0x3c/0x15c
> > [??? 6.784319]? __driver_attach+0x90/0x19c
> > [??? 6.784322]? bus_for_each_dev+0x7c/0xdc
> > [??? 6.794520]? driver_attach+0x24/0x30
> > [??? 6.794523]? bus_add_driver+0xe4/0x208
> > [??? 6.794526]? driver_register+0x5c/0x124
> > [??? 6.802586]? __spi_register_driver+0xa4/0xe4
> > [??? 6.802589]? goodix_berlin_spi_driver_init+0x20/0x1000 [goodix_berlin_spi]
> > [??? 6.802591]? do_one_initcall+0x80/0x1c8
> > [??? 6.902310]? do_init_module+0x60/0x218
> > [??? 6.921988]? load_module+0x1bcc/0x1d8c
> > [??? 6.925847]? init_module_from_file+0x88/0xcc
> > [??? 6.930238]? __arm64_sys_finit_module+0x1dc/0x2e4
> > [??? 6.935074]? invoke_syscall+0x48/0x114
> > [??? 6.938944]? el0_svc_common.constprop.0+0xc0/0xe0
> > [??? 6.943781]? do_el0_svc+0x1c/0x28
> > [??? 6.947195]? el0_svc+0x34/0xd8
> > [??? 6.950348]? el0t_64_sync_handler+0x120/0x12c
> > [??? 6.954833]? el0t_64_sync+0x190/0x194
> > [??? 6.958600] Code: 2a0203f5 2a0303f6 a90363f7 aa0003f7 (b9401c2
> > 
> > Reverting? 8cc3bad9d9d6 ("spi: Remove unneded check for orig_nents") removes the crash.
> > 
> > > 
> > > Meanwhile, I have three changes I posted in the replies to the initial report,
> > > can you combine them all and test? This will be a plan B (? or A, depending on
> > > the culprit).
> > > 
> > 
> > I'll try to apply them and test.
> 
> I stacked the 3 changes, and it works:
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD

Thank you!

I will try to develop and submit a fix against IOMMU which I believe is the
correct place to address this. So, this one will be plan B in case the IOMMU
folks will refuse the other one.

-- 
With Best Regards,
Andy Shevchenko