Date:   Fri, 10 Mar 2023 16:59:17 +0800
From:   "Ye, Xiang" <xiang.ye@intel.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC:     Oliver Neukum <oneukum@suse.com>, Arnd Bergmann <arnd@arndb.de>,
        "Matthias Kaehlcke" <mka@chromium.org>, Lee Jones <lee@kernel.org>,
        Wolfram Sang <wsa@kernel.org>,
        Tyrone Ting <kfting@nuvoton.com>,
        Mark Brown <broonie@kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        "Bartosz Golaszewski" <brgl@bgdev.pl>, <linux-usb@vger.kernel.org>,
        <linux-i2c@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-spi@vger.kernel.org>, <linux-gpio@vger.kernel.org>,
        <srinivas.pandruvada@intel.com>, <heikki.krogerus@linux.intel.com>,
        <andriy.shevchenko@linux.intel.com>,
        <sakari.ailus@linux.intel.com>, <zhifeng.wang@intel.com>,
        <wentong.wu@intel.com>, <lixu.zhang@intel.com>
Subject: Re: [PATCH v4 2/5] gpio: Add support for Intel LJCA USB GPIO driver
Message-ID: <ZArxZZfnlqj6RsGC@ye-NUC7i7DNHE>
References: <20230309071100.2856899-1-xiang.ye@intel.com>
 <20230309071100.2856899-3-xiang.ye@intel.com>
 <2865f3d0-428b-0df1-fc50-f6af3cb9dac3@suse.com>
 <ZAqyhXt9nNIE9Ej7@ye-NUC7i7DNHE>
 <ZArYCD0r6n0sJ7LI@kroah.com>
 <ZArem5MwWrgOY2nJ@ye-NUC7i7DNHE>
 <ZArh5iMkdj0L9AXZ@kroah.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZArh5iMkdj0L9AXZ@kroah.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?k4ZTMl/0GekHQUcIEjCbhBXQNWExK74YXzzFVZ38SgrtbNDY00k7Uiiua2a5?=
 =?us-ascii?Q?I+obK1/tgB/xrUXySvsMK5wPcGiRvc+p8SYlveQqERUlwViiy1rtB284TeGa?=
 =?us-ascii?Q?jmgr0rlXdeK22Z3D4QAqxaFqEimLRRndsCnQfZkbyHVSEp1JP/MFh7p0ND5p?=
 =?us-ascii?Q?jf3bKoi+tMmJOnJQO3lbz+pbJl70+UuZm+SHLYpgcoKNkVuagDmYtqAflGxi?=
 =?us-ascii?Q?wtnobAXE82iBKwA5n57HV32KAf7kFBQLBoTtUadZES2OvFwwjN53Lg80a2Ls?=
 =?us-ascii?Q?89gC6xmB1dNUBsv2BqNrfeC8ASFYY4xj82W+oiB8UApVYQYZABnkRw/eisH1?=
 =?us-ascii?Q?pcW+YG4CDTbuQusyi8oVxNUoKRzv1YTkZeJiJxXvqFpj78FS5ElgJ67QnaNL?=
 =?us-ascii?Q?KvJwBKzHMQX3QV7GQSCt26GlrenCQxbPOMIs/OMnwJugNnCzX2PUEVxTOpnj?=
 =?us-ascii?Q?m6N9vdFWQsn+E3z+trGhOXak/mY74YHAjjbyGpFbr3iPC0726eK3lLVQjPWD?=
 =?us-ascii?Q?Wwd/BT4QWJ3xNFHUcRaY0h10CIDijLSOY0lYv9BPN202O7UqyoLmUlkvYw9c?=
 =?us-ascii?Q?iVx+Is0W2XgAL6L+3f9eY3qdA0qVveEzva4fNNlE0JUltB+LslzpaRaWzY+5?=
 =?us-ascii?Q?u+o137+5iBgNmej3GNijefZrCtIebfIdebv/SZuVMHllaK5Enm2HeRpHTRik?=
 =?us-ascii?Q?pmRmPoxla33obl+HplTsaRVdO1pf3ZAsL02Cn9rRe21SL0DD3bEPA0tGwmjc?=
 =?us-ascii?Q?nG6T139mPMH0tD338ug/1y4jtw2aQ//VK9sA0DH8oohjhBP6yIylU5RRmWt0?=
 =?us-ascii?Q?u8cSfkvFTHp6mvyZkk2N1y2FeI7hqqUKcJLuwT68+lwqYkly7GJPbLBNaVvI?=
 =?us-ascii?Q?n0kSzrOr8HaYHuL0FdRpATzzVPU/g9xCUpIKbs+DEIIS+xJ9NdckdL8VAZX7?=
 =?us-ascii?Q?bqVctbGx+NnXkxh16lIqsN5hWAVM6Vsy4ivNBCCeV7zHdLeSDW7vkWaOHeFz?=
 =?us-ascii?Q?E4MCldrZBeMBZJFSnD5f0NNJfKgh/5xL42GEZqic7uqWCrq+l9I3GLqC7KAc?=
 =?us-ascii?Q?BTCkkuZZYYHVguE9kcMa7zvAGT81dsSwp1X1cfxX/eTmG2cEdEg86qVxAs2M?=
 =?us-ascii?Q?n96TwiJguRoL9UfzjH8Uo+yE3QwHf7kAopteAEgtX8nvRFwS4XB3BWizhuAx?=
 =?us-ascii?Q?zxbsoEEeM5r/ORbVJCs9+Y5Cdq6l4vlECTrQOHzPkl5+kOP4w8+O+uDITPSX?=
 =?us-ascii?Q?gk28TVfs5tv/6i6L2orvJZTeM7XzbyYSAsgN+2gASE4ljIsu5lrkLYO48V1A?=
 =?us-ascii?Q?62BfgzQraEKgYw2iP36dJTx/AQaGtv9Kts7F+0haxeyB1lsX8YHzM1/JizzU?=
 =?us-ascii?Q?CowSZ65QUQJgDS+3mwcsZDWwUFpjsDQw0i6p8SCwBmQF1ILQJlOmzvojLPjV?=
 =?us-ascii?Q?kGzb2lKIzVwU9g/KYNrp+5+LUtAOho85lr5j6oMCiA3iBz2kNr+SsDqkJnpt?=
 =?us-ascii?Q?X4HcZi7HW4ENVZpmPNuwTxwmBLm30pYupCl3FJgFr8iTdbJmxGSSMQWyNJaN?=
 =?us-ascii?Q?V9BJ92Ifvn3s4Z0red0g+aHDeQGLO0bK3lgpMRQE?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3324deab-42a9-434c-c873-08db2145bf99
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1418.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Mar 2023 08:59:25.6543
 (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: LXTxBeS3CilxQqhKnMnSlBNQXNCb93PN1/YpV85zKAJf6uAr5c2UxISx3Ho2t730IeaFG2+IovO/LvnLcR68dA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7624
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Mar 10, 2023 at 08:53:10AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Mar 10, 2023 at 03:39:07PM +0800, Ye, Xiang wrote:
> > On Fri, Mar 10, 2023 at 08:11:04AM +0100, Greg Kroah-Hartman wrote:
> > > On Fri, Mar 10, 2023 at 01:01:11PM +0800, Ye, Xiang wrote:
> > > > On Thu, Mar 09, 2023 at 02:40:10PM +0100, Oliver Neukum wrote:
> > > > > 
> > > > > 
> > > > > On 09.03.23 08:10, Ye Xiang wrote:
> > > > > 
> > > > > > +#define LJCA_GPIO_BUF_SIZE 60
> > > > > > +struct ljca_gpio_dev {
> > > > > > +	struct platform_device *pdev;
> > > > > > +	struct gpio_chip gc;
> > > > > > +	struct ljca_gpio_info *gpio_info;
> > > > > > +	DECLARE_BITMAP(unmasked_irqs, LJCA_MAX_GPIO_NUM);
> > > > > > +	DECLARE_BITMAP(enabled_irqs, LJCA_MAX_GPIO_NUM);
> > > > > > +	DECLARE_BITMAP(reenable_irqs, LJCA_MAX_GPIO_NUM);
> > > > > > +	u8 *connect_mode;
> > > > > > +	/* mutex to protect irq bus */
> > > > > > +	struct mutex irq_lock;
> > > > > > +	struct work_struct work;
> > > > > > +	/* lock to protect package transfer to Hardware */
> > > > > > +	struct mutex trans_lock;
> > > > > > +
> > > > > > +	u8 obuf[LJCA_GPIO_BUF_SIZE];
> > > > > > +	u8 ibuf[LJCA_GPIO_BUF_SIZE];
> > > > > 
> > > > > And here we have a violation of DMA coherency rules.
> > > > > Basically you cannot embed buffers into other data structures
> > > > > if they can be subject to DMA.
> > > > But obuf and ibuf does not used to do DMA transfer here.
> > > > It is actually copied from or to ljca buffer to do URB transfer.
> > > 
> > > urb transfers _ARE_ DMA transfers.
> > > 
> > > > Should it still need to follow the DMA coherency rules?
> > > 
> > > Yes, all buffers for USB urbs are required to follow those rules.
> > But these two buffers are not used to do USB urb transfer directly.
> > For the "u8 obuf[LJCA_GPIO_BUF_SIZE]",  it will be copied to ljca buffer
> > ("header->data" as below code [1] showed) first. Then the "header" is used
> > to do the actual urb transfer.
> > 
> > And the "header" is allocated by using kmalloc. It should has met the DMA
> > coherency rules.
> > 
> > [1] """
> > struct ljca_msg *header;
> > ...
> > header = kmalloc(msg_len, GFP_KERNEL);
> > if (!header)
> > 	return -ENOMEM;
> 
> Ok, that's good, but why have 2 buffers for this then?
"u8 obuf[LJCA_GPIO_BUF_SIZE]" is used as a out buffer of LJCA GPIO
package.
"u8 ibuf[LJCA_GPIO_BUF_SIZE]" is used as a in buffer of LJCA GPIO
package.

Both obuf and ibuf here are not used to do URB transfer directly.

This obuf and ibuf somtimes are using at the same time when calling
ljca_transfer. So we need these two buffers.

--
Thanks
Ye Xiang