Received-SPF: pass (google.com: domain of linux-kernel+bounces-166819-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 2 May 2024 20:01:50 +0200
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
To: linux-renesas-soc@vger.kernel.org
Cc: Dirk Behme <dirk.behme@de.bosch.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Subject: Re: [RFC PATCH 1/2] serial: sh-sci: start hrtimer after setting up
 DMA
Message-ID: <20240502180150.r3pb2izsjlpqzszz@ninjato>
Mail-Followup-To: Wolfram Sang <wsa+renesas@sang-engineering.com>,
	linux-renesas-soc@vger.kernel.org,
	Dirk Behme <dirk.behme@de.bosch.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
References: <20240416123545.7098-4-wsa+renesas@sang-engineering.com>
 <20240416123545.7098-5-wsa+renesas@sang-engineering.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="js57najwoohyptm3"
Content-Disposition: inline
In-Reply-To: <20240416123545.7098-5-wsa+renesas@sang-engineering.com>


--js57najwoohyptm3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Apr 16, 2024 at 02:35:47PM +0200, Wolfram Sang wrote:
> In the RX DMA completion handler, the hrtimer was restarted before DMA
> was set up. If DMA failed, for some reason, it would clean up and the
> hrtimer would run into a NULL-pointer. Restart the timer after DMA was
> successfully set up.

Further investigations, please review:

Originally, I thought this was the race condition Dirk encountered. But
I didn't take locking into account. sci_dma_rx_timer_fn() is protected
by the uart_port_lock. sci_dma_rx_complete() is also protected by the
uart_port_lock. So, the position of restarting the hrtimer should not
matter.

However, I still think it is good coding practice (and easier to
understand) if we cancel the hrtimer at the begin of
sci_dma_rx_complete() and reenable it only if setting DMA was
successful. Because that basically means the timer only runs when DMA
was scheduled and has not finished yet.

There is some unnecessary unlock/lock in the error handling of
sci_dma_rx_complete(). I'll simplify this by moving the dev_err
downwards.


--js57najwoohyptm3
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAmYz1Q4ACgkQFA3kzBSg
KbZm0w//eZ5cZq4DX8pPTTvVIVuT9AjpiB59BUYA79fhrfvqyyCVCBp723SSdNJT
7O+8oUCVy+jpkDb0CTCFYy2ejvEwDVBoq5TMgRREpGw04MnBzSvYmogeLhdKdqro
JCTuIdoU4K3OzXZsbqLi98ZQwpZmXaxNPF4Hjq3hQN+atJu9cmIgQq/2jbNptx7e
Cq9ow1Jowp1RngMR9i0CVypjwwz4FsPk2u8j9jhrv1+Jnj68ThkoTePx0wof2ky0
kYS6uNd+7P5MWVcLuFRCXAcPNNAxm4VgIRdd2qNJZ7G1v49uDwJ5IKlDlem5i3XQ
WpyXOx0RizXXxlDbho+eQi6WgaafJ9q8KgcqE5iIBkp4gPp6oZ+2sd0bTUT2Xu97
deDVQPoh1oiyY9T3reM50dRZ43zp/Yrt6Q8WC+zK7V6mNfuWQ6upN4umbOXPObCH
t3Si8GOtMEox8fhg96hbbw3qNDPMv85Vr11dGGXmwOc6VY7WA61B299blH82gacT
tPaa1oYuo7cm4yGc3ErqrwEs8a7rplctQNiE9YkTfIfhmysXiSjN4QwB6spxq3Ps
09Rgews6i6y+Lzez1/CTn7xEvXWKoPoYNFOYr/wnkE81dOqcTvFT3ca3mUFLBtNv
i0y/k12/IuQGweQn2G0lx5A4ahvtrGIKAzW28dHeJoVj7T6wq3c=
=4p+v
-----END PGP SIGNATURE-----

--js57najwoohyptm3--