Re: [PATCH v4 2/9] bus: mhi: Move sahara protocol driver under drivers/bus/mhi

[Kishore Batta <kishore.batta@oss.qualcomm.com>]

On 4/10/2026 1:50 AM, Jeff Hugo wrote:
> On 3/19/2026 12:31 AM, Kishore Batta wrote:
>> The Sahara protocol driver is currently located under the QAIC
>> accelerator subsystem even though protocol itself is transported over 
>> the
>> MHI bus and is used by multiple Qualcomm flashless devices.
>>
>> Relocate the Sahara protocol driver to drivers/bus/mhi and register 
>> it as
>> an independent MHI protocol driver. This avoids treating Sahara as QAIC
>> specific and makes it available for reuse by other MHI based devices.
>>
>> As part of this move, introduce a dedicated Kconfig and Makefile 
>> under the
>> MHI subsystem and expose the sahara interface via a common header.
>
> I don't think this belongs under MHI. Mani needs to confirm that he 
> agrees with the concept of moving this there.
>
> The Sahara protocol as defined by the spec does not require MHI. We 
> know that there are Sahara implementations over USB. I don't see a 
> dependency or relationship to MHI other than the current in-kernel 
> implementation uses MHI, but there are plenty of things that use MHI 
> (qaic, mhi-net, ath12k, etc) which are not a part of the MHI bus.
>
> The implementation presented in this series is not well integrated 
> into MHI, which also suggests to me that it doesn't belong there. The 
> Documentation is not integrated with MHI (which I mentioned over on 
> that patch) and I see the header file (sahara.h) is also not integrated.


On Patch 1 series, I have got a review comment from Bjorn to have it 
moved to drivers/bus/mhi as this implementation only supports MHI and is 
going to be used with MHI. Hence the sahara driver is moved to this 
directory. Do i need to move this to a different directory ? I will move 
the Sahara documentation once we conclude on where Sahara driver needs 
to be moved.

>
>> diff --git a/drivers/accel/qaic/qaic_drv.c 
>> b/drivers/accel/qaic/qaic_drv.c
>> index 
>> 63fb8c7b4abcbe4f1b76c32106f4e8b9ea5e2c8e..76cc8086825e7949ed756d51fcb56a08f392d228 
>> 100644
>> --- a/drivers/accel/qaic/qaic_drv.c
>> +++ b/drivers/accel/qaic/qaic_drv.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/msi.h>
>>   #include <linux/mutex.h>
>>   #include <linux/pci.h>
>> +#include <linux/sahara.h>
>
> What do we need this for? register()/unregister() get removed.

ACK.I will remove it.
>
>>   #include <linux/spinlock.h>
>>   #include <linux/workqueue.h>
>>   #include <linux/wait.h>
>> @@ -32,7 +33,6 @@
>>   #include "qaic_ras.h"
>>   #include "qaic_ssr.h"
>>   #include "qaic_timesync.h"
>> -#include "sahara.h"
>>     MODULE_IMPORT_NS("DMA_BUF");
>>   @@ -782,18 +782,12 @@ static int __init qaic_init(void)
>>       ret = pci_register_driver(&qaic_pci_driver);
>>       if (ret) {
>>           pr_debug("qaic: pci_register_driver failed %d\n", ret);
>> -        return ret;
>> +        goto free_pci;
>
> This is wrong, and there should not be a change here.


ACK. I will revert this change.

>
>>         ret = mhi_driver_register(&qaic_mhi_driver);
>>       if (ret) {
>>           pr_debug("qaic: mhi_driver_register failed %d\n", ret);
>> -        goto free_pci;
>> -    }
>> -
>> -    ret = sahara_register();
>> -    if (ret) {
>> -        pr_debug("qaic: sahara_register failed %d\n", ret);
>>           goto free_mhi;
>
> This is also wrong


I want to make sure I understand the concern correctly before changing 
the init flow. With Sahara now being a standalone MHI client driver 
(moved out of QAIC and self registering), QAIC no longer calling 
sahara_register() is intentional. However, removing that call also 
removes an implicit init time dependency/failure gate. Is the objection 
primarily about losing that gating / ordering guarantee or is there 
another issue with removing the call itself ?

Once i understand that, I'll update the patches in the direction you prefer.

>
>>   @@ -847,7 +841,6 @@ static void __exit qaic_exit(void)
>>       qaic_ras_unregister();
>>       qaic_bootlog_unregister();
>>       qaic_timesync_deinit();
>> -    sahara_unregister();
>>       mhi_driver_unregister(&qaic_mhi_driver);
>>       pci_unregister_driver(&qaic_pci_driver);
>>   }
>> diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig
>> index 
>> b39a11e6c624ba00349cca22d74bd876020590ab..4acedb886adccc6f76f69c241d53106da59b491f 
>> 100644
>> --- a/drivers/bus/mhi/Kconfig
>> +++ b/drivers/bus/mhi/Kconfig
>> @@ -7,3 +7,4 @@
>>     source "drivers/bus/mhi/host/Kconfig"
>>   source "drivers/bus/mhi/ep/Kconfig"
>> +source "drivers/bus/mhi/sahara/Kconfig"
>> diff --git a/drivers/bus/mhi/Makefile b/drivers/bus/mhi/Makefile
>> index 
>> 354204b0ef3ae4030469a24a659f32429d592aef..e4af535e1bb1bc9481fae60d7eb347700d2e874c 
>> 100644
>> --- a/drivers/bus/mhi/Makefile
>> +++ b/drivers/bus/mhi/Makefile
>> @@ -3,3 +3,6 @@ obj-$(CONFIG_MHI_BUS) += host/
>>     # Endpoint MHI stack
>>   obj-$(CONFIG_MHI_BUS_EP) += ep/
>> +
>> +# Sahara MHI protocol
>> +obj-$(CONFIG_MHI_SAHARA) += sahara/
>> diff --git a/drivers/bus/mhi/sahara/Kconfig 
>> b/drivers/bus/mhi/sahara/Kconfig
>> new file mode 100644
>> index 
>> 0000000000000000000000000000000000000000..3f1caf6acd979a4af68aaf0e250aa54762e8cda5
>> --- /dev/null
>> +++ b/drivers/bus/mhi/sahara/Kconfig
>> @@ -0,0 +1,18 @@
>> +config MHI_SAHARA
>> +    tristate
>> +    depends on MHI_BUS
>> +    select FW_LOADER_COMPRESS
>
> Why are we selecting this? I don't see anyone else doing this. Sahara 
> should work with and without firmware compression.

ACK. I will remove this.


>
>> +    select FW_LOADER_COMPRESS_XZ
>> +    select FW_LOADER_COMPRESS_ZSTD
>> +    help
>> +      Enable support for the Sahara protocol transported over the 
>> MHI bus.
>> +
>> +      The Sahara protocol is used to transfer firmware images, retrieve
>> +      memory dumps and exchange command mode DDR calibration data 
>> between
>> +      host and device. This driver is not tied to a specific SoC and 
>> may be
>> +      used by multiple MHI based devices.
>> +
>> +      If unsure, say N.
>> +
>> +      To compile this driver as a module, choose M here: the module 
>> will be
>> +      called mhi_sahara.
>> diff --git a/drivers/bus/mhi/sahara/Makefile 
>> b/drivers/bus/mhi/sahara/Makefile
>> new file mode 100644
>> index 
>> 0000000000000000000000000000000000000000..fc02a25935011cbd7138ea8f24b88cf5b032a4ce
>> --- /dev/null
>> +++ b/drivers/bus/mhi/sahara/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-$(CONFIG_MHI_SAHARA) += mhi_sahara.o
>> +mhi_sahara-y := sahara.o
>> diff --git a/drivers/accel/qaic/sahara.c 
>> b/drivers/bus/mhi/sahara/sahara.c
>> similarity index 99%
>> rename from drivers/accel/qaic/sahara.c
>> rename to drivers/bus/mhi/sahara/sahara.c
>> index 
>> fd3c3b2d1fd3bb698809e6ca669128e2dce06613..8ff7b6425ac5423ef8f32117151dca10397686a8 
>> 100644
>> --- a/drivers/accel/qaic/sahara.c
>> +++ b/drivers/bus/mhi/sahara/sahara.c
>> @@ -1,6 +1,8 @@
>> -// SPDX-License-Identifier: GPL-2.0-only
>> -
>> -/* Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights 
>> reserved. */
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
>> + *
>> + */
>
> What makes you think that changing the copyright markings is 
> appropiate when moving a file?
>
> Furthermore, I wrote this code from scratch based on the spec document 
> and therefore know beyond a doubt that this file did not exist prior 
> to 2024, so what you are changing the markings to is completely invalid.
>
> Also the SPDX marking you are using is long deprecated and should not 
> be used.


Sorry. I misunderstood the review comment that was given on v1 patchset. 
Only the copyright style needs to match the subsystem where this driver 
is moved to. I will make the changes in the next patch set.

>
>>   #include <linux/devcoredump.h>
>>   #include <linux/firmware.h>
>> @@ -9,12 +11,11 @@
>>   #include <linux/minmax.h>
>>   #include <linux/mod_devicetable.h>
>>   #include <linux/overflow.h>
>> +#include <linux/sahara.h>
>>   #include <linux/types.h>
>>   #include <linux/vmalloc.h>
>>   #include <linux/workqueue.h>
>>   -#include "sahara.h"
>> -
>>   #define SAHARA_HELLO_CMD        0x1  /* Min protocol version 1.0 */
>>   #define SAHARA_HELLO_RESP_CMD        0x2  /* Min protocol version 
>> 1.0 */
>>   #define SAHARA_READ_DATA_CMD        0x3  /* Min protocol version 
>> 1.0 */
>> @@ -928,8 +929,13 @@ int sahara_register(void)
>>   {
>>       return mhi_driver_register(&sahara_mhi_driver);
>>   }
>> +module_init(sahara_register);
>>     void sahara_unregister(void)
>>   {
>>       mhi_driver_unregister(&sahara_mhi_driver);
>>   }
>> +module_exit(sahara_unregister);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Qualcomm Sahara MHI protocol driver");
>> diff --git a/drivers/accel/qaic/sahara.h b/include/linux/sahara.h
>> similarity index 100%
>> rename from drivers/accel/qaic/sahara.h
>> rename to include/linux/sahara.h
>>
>