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;
Message-ID: <017716b5-d36c-53fa-9ffe-61bf9acb846d@suse.com>
Date:   Tue, 20 Jun 2023 14:11:10 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v8 1/6] usb: Add support for Intel LJCA device
To:     "Wu, Wentong" <wentong.wu@intel.com>,
        Oliver Neukum <oneukum@suse.com>,
        "Ye, Xiang" <xiang.ye@intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        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>,
        Marc Zyngier <maz@kernel.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "Pandruvada, Srinivas" <srinivas.pandruvada@intel.com>,
        "heikki.krogerus@linux.intel.com" <heikki.krogerus@linux.intel.com>,
        "andriy.shevchenko@linux.intel.com" 
        <andriy.shevchenko@linux.intel.com>,
        "sakari.ailus@linux.intel.com" <sakari.ailus@linux.intel.com>,
        "Wang, Zhifeng" <zhifeng.wang@intel.com>,
        "Zhang, Lixu" <lixu.zhang@intel.com>
References: <20230511175844.185070-1-xiang.ye@intel.com>
 <20230511175844.185070-2-xiang.ye@intel.com>
 <ca61c02d-9a3b-8e75-664f-c86683322e8e@suse.com>
 <DM6PR11MB4316170659C5F22C7E54EF808D5CA@DM6PR11MB4316.namprd11.prod.outlook.com>
Content-Language: en-US
From:   Oliver Neukum <oneukum@suse.com>
In-Reply-To: <DM6PR11MB4316170659C5F22C7E54EF808D5CA@DM6PR11MB4316.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aFpzOWpDRm9rMzg4UGw4QnZKbzhsMUhzQXJkMzN3WVdBZW84Q0o4TXFhRXZN?=
 =?utf-8?B?ZjB1bE1FbWV0dG83ZzVZVGZCWTRJRGZzN0dCU0NGZ0JHUm1Pa3FCYmk4Snhr?=
 =?utf-8?B?RU81bjZ0ZFZ3bUZiaDZrU2FJNXFjN3F2dnJ0eHlTekVmcGVXWS9JVDVnekhz?=
 =?utf-8?B?eHFxRkpLem9KVnYvbHd1bmNkd25FU1lXWFNjUTFaNW5adHlUUmx3MEZsc2RQ?=
 =?utf-8?B?cFBxaDB2YlpiSmtLT2d0M01ZSk1mTjd4WWNsQmtIdUFRcHVtdkt0bE9oVUth?=
 =?utf-8?B?R0lrTHg3ZEppcDNXN0hGazVxcjBIcDNZOHlmU1RwMGVXRlpTZGtKekhTVnZp?=
 =?utf-8?B?SjAwTkxnVEpUVEtNU1RQMGJFdWVWQkVEbmNuanVOQi9BSnlhYXNjVkhLbi9N?=
 =?utf-8?B?c1ZQWGRVbWMwdWZHZEJXa2hKc1hKM0dHQ2YvSlh4VnRYVEI0b3lBZWF4K3lj?=
 =?utf-8?B?T3VDTWpNZkQ0Ynp1blV2THNxbjRTS1BhbHN3NEt5Y0t1WDd3ZS9leWpKWE5F?=
 =?utf-8?B?RWhGdlY1TEs2bC91bU9IQUZ3VEdLZkJreXdyTnpDREk5bTBTZjQ2RDV1MUdS?=
 =?utf-8?B?RXlIRXZSQzFUM0JySEZhZGx5a1NXSGk4ZkpvSWQ4QlIwcWorYmFrLytBQnRz?=
 =?utf-8?B?cUVNZHI0NjFPYTB5Nk82M3RveE9vSXVsSXVqV1pueWNuT0ozRVptQlNuZFl2?=
 =?utf-8?B?QkVxUmZRc3dKbjh5bFdwTDRZYVNwMVM3LzI3OSs4SWZlRmNORnA4bHMzZmlK?=
 =?utf-8?B?cVRXSWV0aDUxY2hyeEtQTEVsUW1MU1JHUW1jWllpZW9pcVNyRms2QlFyY0pE?=
 =?utf-8?B?SXl6dGJ0MVVEZkRvZXl3NjI3UG5hbkxUS1hRZXQ2WjA5ei9NRWhNNC9OeDZi?=
 =?utf-8?B?M0dBbGNtb2pqdklqdU01ck0zRWxQaTUwNDd0Z09HR3ljQ2lRVzg0SEpEeGly?=
 =?utf-8?B?NEFXdElzdWpVcFptbjk4eUJrYVl4QUFoSWoxU3ByUDkwRk1XRi9lZk1hOFRX?=
 =?utf-8?B?ZnlJZGxEOE5KOCtUZjdvZnBXa1BnN3BHTm1KWXZmV290eldwdHM3d1gvb3ZJ?=
 =?utf-8?B?YVJqL1k4U1VrSHFPL21aUTFpWUQyYkU1cnRVT1lkMkF2OEVlY1BOb21kUjY0?=
 =?utf-8?B?T3hER3BoYUR0RUJueEE2OEdWRWtNczljdVFBbXlHK2JWMjFlQjZScGEyRUMw?=
 =?utf-8?B?SDJJTUtQMEJmQlhFYzdZY3FaOHdTUGJYNy9rT3dRMTFpWGdXcllnMkNiS2RE?=
 =?utf-8?B?aEdFVUJadU5qbE9zSWFMTVpyQlM3ZEdqcTczWnFNTGpKdVIyaGttQ2R5c3Z5?=
 =?utf-8?B?dG1ucmpoaFhSandwQjNVdzRSTVRiWkp0TXJSc0N5ZUtHQkVyRXNSNDNiQjdX?=
 =?utf-8?B?MGptUFRtVFVmRjZEb0JhWkxmd1NPUHBWR1FaWFNJUW53SzBRbkRYT1hlRnB4?=
 =?utf-8?B?QzFLaXpTYUkzMHNrRGpWNlErREZCdlNPRnZCZ252TDBvK0xsN0s2d3VaUU1H?=
 =?utf-8?B?WnB2RVdOOGpvNis3Z2ZzTSsrVEVPR2NoOUt2SE9aYVp2Z3FucTBKVnZKQmFo?=
 =?utf-8?B?VWtuMXNPMGEvYVdiNFNqSnVwUXc5d0NOeDdBNHUrUTBFdk9XUWUrYlN4NkFO?=
 =?utf-8?B?Sk9URHlXdFRWZGw4NTZuby9pcVEzNVgzQU0zcENZRzhvcTlyZDNweUxsTDIx?=
 =?utf-8?B?N0hVdXlYOVVTWDZtaVFyWGRxbnloS2pIeTZON1BNRmlJa3hndUhvTnV1eFZn?=
 =?utf-8?B?dVpURTlUK3ZwZ2lkWDlaMi9TVmR2a3JHUHhkR0RkbDdBd0thdmd4d3VUUFds?=
 =?utf-8?B?ZVpzekZrVy96MHc0WVVMbWJMQWc2SGJWcldLcGk2Mjd5SVcxY2dnTkNld0xt?=
 =?utf-8?B?MUhaK1FrNGFnRlBHTmV6eE1xWFJXakJaZ2NnekZjZnI1M3lZWEorWGRPL1Vk?=
 =?utf-8?B?SWZHS3B3Y2Z1SVlEc3JWbnhwNkRMcnh1WFNsaVlRL2tPMzFCM0pnV1RMdW1a?=
 =?utf-8?B?YlhTU1o3ZnJUbi9iWUcrUUlZRkorbkZWQzdBTXNab3I1c2xUZ1pIUlVPQ3lw?=
 =?utf-8?B?L2NVaUZka1dRSlJWM0l5bkxPTjFWeStmZjY4alo1a3JVdDR6OWFSVTF5bmp0?=
 =?utf-8?B?ZzNaTDNLRjZRcUFTYW5SVmxhQndubDljRW9nOXF3T1oxU3lDVkJ0MFlVRkdi?=
 =?utf-8?Q?GyOyu/Pyly50Q9ER0ShUZ2U=3D?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 123e7359-b9fd-4b35-8287-08db71877188
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB7104.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jun 2023 12:11:14.4083
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: VtCPlBSIpiyla3Anw2eJsNJUY7d6smECoqD0ShdWa7eouhVWlSwWlgTXZazZFqAM0RfvcaDf/a3Ak1u+tUc4/Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU0PR04MB9660
X-Spam-Status: No, score=-0.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,RCVD_IN_VALIDITY_RPBL,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no 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 20.06.23 09:39, Wu, Wentong wrote:

Hi,

>>> +
>>> +	ibuf_len = READ_ONCE(stub->ipacket.ibuf_len);
>>> +	ibuf = READ_ONCE(stub->ipacket.ibuf);
>>
>> This races against stub_write(). Yes, every value now is consistent within this
>> function. But that does not solve the issue. The pair needs to be consistent. You
>> need to rule out that you are reading a different length and buffer location. I am
>> afraid this needs a spinlock.
> 
> Sorry, based on my understanding, the stub's ibuf_len and ibuf won't be changed by
> stub_write until firmware ack the current command.

Or a timeout.
There are two places in the driver ibuf_len and ibuf are touched.
Both are in ljca_stub_write(), but in case of a timeout they are set
to NULL. And you are checking for that case. That is good, but not sufficient,
as READ_ONCE does not guarantee ordering, nor do you use WRITE_ONCE to set them.

> This memory barrier here is for potential cache consistency issue in some arches though
> most likely this driver will only be run on X86 platform.

Well, you can move them to architecture dependent directories. But if not, they
will have to work generically.

A correct version would be something like:

+static int ljca_parse(struct ljca_dev *dev, struct ljca_msg *header)
+{
+	struct ljca_stub *stub;
+	unsigned int *ibuf_len;
+	u8 *ibuf;
+
+	stub = ljca_stub_find(dev, header->type);
+	if (IS_ERR(stub))
+		return PTR_ERR(stub);
+
+	if (!(header->flags & LJCA_ACK_FLAG)) {
+		ljca_stub_notify(stub, header->cmd, header->data, header->len);
+		return 0;
+	}
+
+	if (stub->cur_cmd != header->cmd) {
+		dev_err(&dev->intf->dev, "header and stub current command mismatch (%x vs %x)\n",
+			header->cmd, stub->cur_cmd);
+		return -EINVAL;
+	}
+

smp_rmb();

+	ibuf_len = READ_ONCE(stub->ipacket.ibuf_len);
+	ibuf = READ_ONCE(stub->ipacket.ibuf);
+
+	if (ibuf && ibuf_len) {
+		unsigned int newlen;
+
+		newlen = min_t(unsigned int, header->len, *ibuf_len);
+
+		*ibuf_len = newlen;
+		memcpy(ibuf, header->data, newlen);
+	}
+
+	stub->acked = true;
+	wake_up(&dev->ack_wq);
+
+	return 0;
+}
+
+static int ljca_stub_write(struct ljca_stub *stub, u8 cmd, const void *obuf, unsigned int obuf_len,
+			   void *ibuf, unsigned int *ibuf_len, bool wait_ack, unsigned long timeout)
+{
+	struct ljca_dev *dev = usb_get_intfdata(stub->intf);
+	u8 flags = LJCA_CMPL_FLAG;
+	struct ljca_msg *header;
+	unsigned int msg_len = sizeof(*header) + obuf_len;
+	int actual;
+	int ret;
+
+	if (msg_len > LJCA_MAX_PACKET_SIZE)
+		return -EINVAL;
+
+	if (wait_ack)
+		flags |= LJCA_ACK_FLAG;
+
+	header = kmalloc(msg_len, GFP_KERNEL);
+	if (!header)
+		return -ENOMEM;
+
+	header->type = stub->type;
+	header->cmd = cmd;
+	header->flags = flags;
+	header->len = obuf_len;
+
+	if (obuf)
+		memcpy(header->data, obuf, obuf_len);
+
+	dev_dbg(&dev->intf->dev, "send: type:%d cmd:%d flags:%d len:%d\n", header->type,
+		header->cmd, header->flags, header->len);
+
+	usb_autopm_get_interface(dev->intf);
+	if (!dev->started) {
+		kfree(header);
+		ret = -ENODEV;
+		goto error_put;
+	}
+
+	mutex_lock(&dev->mutex);
+	stub->cur_cmd = cmd;

WRITE_ONCE(stub->ipacket.ibuf, ibuf);
WRITE_ONCE(stub->ipacket.ibuf_len, ibuf_len);

+	stub->acked = false;

smp_wmb();

[..]

+error_unlock:

WRITE_ONCE(stub->ipacket.ibuf, NULL);
WRITE_ONCE(stub->ipacket.ibuf_len, NULL);
smp_wmb();

+	mutex_unlock(&dev->mutex);

	Regards
		Oliver