IronPort-PHdr: =?us-ascii?q?9a23=3ASpz/mxX5q7LEbdvV530Ih1zkYuvV8LGtZVwlr6E/?=
 =?us-ascii?q?grcLSJyIuqrYbRSOt8tkgFKBZ4jH8fUM07OQ6PGwHzRYqb+681k6OKRWUBEEjc?=
 =?us-ascii?q?hE1ycBO+WiTXPBEfjxciYhF95DXlI2t1uyMExSBdqsLwaK+i764jEdAAjwOhRo?=
 =?us-ascii?q?LerpBIHSk9631+ev8JHPfglEnjSwbLdxIRmssQndqtQdjJd/JKo21hbHuGZDdf?=
 =?us-ascii?q?5MxWNvK1KTnhL86dm18ZV+7SleuO8v+tBZX6nicKs2UbJXDDI9M2Ao/8LrrgXM?=
 =?us-ascii?q?TRGO5nQHTGoblAdDDhXf4xH7WpfxtTb6tvZ41SKHM8D6Uaw4VDK/5KpwVhTmlD?=
 =?us-ascii?q?kIOCI48GHPi8x/kqRboA66pxdix4LYeZyZOOZicq/Ye94RWGhPUdtLVyFZAo2y?=
 =?us-ascii?q?cYUPAOgcM+hWrIfyqEcBoxS5CwmwH+7v1iZIhnDq0aEkz+gsEwfL1xEgEdIUt3?=
 =?us-ascii?q?TUqc34OKkOXOCxyKnH0zPDZO5U1zfg84fHaQ4urf+RVr93bcfRzU0vGB3DjlmK?=
 =?us-ascii?q?q4LlIiia2f8TvGWA8uVvS+avi2o9pw5tpTivw94hh4/UjYwW0lDJ7SR0zYUvKd?=
 =?us-ascii?q?GmRkN3f8SoHIZQuiyVLYd7RsAvT3l1tCs7ybAKo4O3cSgOxZg92RLTdvKKf5KV?=
 =?us-ascii?q?7h/sSeqcJypzimh/d7KlnRmy9FCtyuj7VsapzllHtjFFktzQtnAV0BzT99SHRu?=
 =?us-ascii?q?N9/ki/3TaP0Bje5f9fIU8umqfXMYMuzaA0lpoXqEjDGTL2mFntg6+Ra0Uk/PWn?=
 =?us-ascii?q?5/r9brXnvp+cN4p0iw7kPqsyncy/BPw0MgkIX2eF5eSxzLzu8E7jTLlUgPA6jL?=
 =?us-ascii?q?PVvI3aKMkUvKK0BwxY3p4m6xmlDjem1NoYnWMALFJAYB+Kk4zpO1XQL/D+F/ux?=
 =?us-ascii?q?gUmsnyxwyP/YJbDuH4vCLnzHkLv7Ybl97EtcxBIpzd9D/5JUFq0BIPXrV0/1td?=
 =?us-ascii?q?zYCAI5MgOtz+bkFtp9zIUeVnyLAqKCLqPSvkGH5vg1L+mPeoAVojD9JOYh5/L0?=
 =?us-ascii?q?in85g1AdLuGV2s5dJ1S/Ef1ga2qEZ2bshc0ZHChA9l46S+PjjlKqXTNXe3+uUq?=
 =?us-ascii?q?967TY+XsbuWYPCQJ29xbqd1g+0GZpZYm1JEFeRCWyufIKBDbNEPCaTJNJx1z8J?=
 =?us-ascii?q?T76sT6c/2hy08gz30bxqKqzT4CJO5rz5090grcTXkRw0vRxpBc6QmSnZZmVwk2?=
 =?us-ascii?q?VOZyI81q1XrUF5zhGI1q0u0K8QLsBa+/4cClRyDpXb1eEvTomqAg8=3D?=
From: "Allen Hubbe" <Allen.Hubbe@dell.com>
To: "'Logan Gunthorpe'" <logang@deltatee.com>,
        <linux-ntb@googlegroups.com>, <linux-kernel@vger.kernel.org>
Cc: "'Jon Mason'" <jdmason@kudzu.us>
References: <20171209000217.18366-1-logang@deltatee.com> <20171209000217.18366-2-logang@deltatee.com>
In-Reply-To: <20171209000217.18366-2-logang@deltatee.com>
Subject: RE: [PATCH 2/2] ntb_hw_switchtec: Check for alignment of the buffer in mw_set_trans()
Date: Mon, 11 Dec 2017 09:58:29 -0500
Message-ID: <000201d37290$81605eb0$84211c10$@dell.com>
MIME-Version: 1.0
Content-Type: text/plain;
        charset="utf-8"
Thread-Index: AQHTcIEAmu1N6hj0nUuwK+dCMVjXlaM+OxnQ
Content-Language: en-us
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 1984
Lines: 49

From: Logan Gunthorpe
> With Switchtec hardware, the buffer used for a memory window must be
> aligned to its size (the hardware only replaces the lower bits). In
> certain circumstances dma_alloc_coherent() will not provide a buffer
> that adheres to this requirement like when using the CMA and
> CONFIG_CMA_ALIGNMENT is set lower than the buffer size.
> 
> When we get an unaligned buffer mw_set_trans() should return an error.
> We also log an error so we know the cause of the problem.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Cc: Jon Mason <jdmason@kudzu.us>
> ---
>  drivers/ntb/hw/mscc/ntb_hw_switchtec.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/ntb/hw/mscc/ntb_hw_switchtec.c b/drivers/ntb/hw/mscc/ntb_hw_switchtec.c
> index 709f37fbe232..984b83bc7dd3 100644
> --- a/drivers/ntb/hw/mscc/ntb_hw_switchtec.c
> +++ b/drivers/ntb/hw/mscc/ntb_hw_switchtec.c
> @@ -315,6 +315,19 @@ static int switchtec_ntb_mw_set_trans(struct ntb_dev *ntb, int pidx, int widx,
>  	if (xlate_pos < 12)
>  		return -EINVAL;
> 
> +	if (addr & ((1 << xlate_pos) - 1)) {

!IS_ALIGNED(addr, BIT_ULL(xlate_pos))

> +		/*
> +		 * In certain circumstances we can get a buffer that is
> +		 * not aligned to its size. (Most of the time
> +		 * dma_alloc_coherent ensures this). This can happen when
> +		 * using large buffers allocated by the CMA
> +		 * (see CMA_CONFIG_ALIGNMENT)
> +		 */
> +		dev_err(&sndev->stdev->dev,
> +			"ERROR: Memory window address is not aligned to it's size!\n");

This would be the only ntb hw driver that prints an error in this situation.  The ntb_mw_get_align() should provide enough information to client drivers to determine the alignment requirements before calling ntb_mw_set_trans().  IMO no need to print here, but let's see what others say.

> +		return -EINVAL;
> +	}
> +
>  	rc = switchtec_ntb_part_op(sndev, ctl, NTB_CTRL_PART_OP_LOCK,
>  				   NTB_CTRL_PART_STATUS_LOCKED);
>  	if (rc)
> --
> 2.11.0