| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f5b7216c-8e6f-16c4-6902-dc8a04997fb9@amd.com>
Date: Wed, 19 Jul 2023 18:06:40 +0530
From: "Gupta, Nipun" <nipun.gupta@....com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org, git@....com,
pieter.jansen-van-vuuren@....com, nikhil.agarwal@....com,
michal.simek@....com, abhijit.gangurde@....com
Subject: Re: [PATCH] cdx: add support for bus mastering
On 7/18/2023 7:16 PM, Greg KH wrote:
> On Tue, Jul 18, 2023 at 03:36:51PM +0530, Nipun Gupta wrote:
>> Introduce cdx_set_master() and cdx_clear_master() APIs
>> to support enable and disable of bus mastering. Drivers
>> need to use these APIs to enable/disable DMAs from the
>> CDX devices.
>
> You do have a full 72 columns, why not use that?
sure, will update accordingly.
>
>> Signed-off-by: Nipun Gupta <nipun.gupta@....com>
>> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@....com>
>> ---
>> drivers/cdx/cdx.c | 32 ++++++++++++++
>> drivers/cdx/controller/cdx_controller.c | 4 ++
>> drivers/cdx/controller/mcdi_functions.c | 57 +++++++++++++++++++++++++
>> drivers/cdx/controller/mcdi_functions.h | 13 ++++++
>> include/linux/cdx/cdx_bus.h | 16 +++++++
>> 5 files changed, 122 insertions(+)
>>
>> diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
>> index d2cad4c670a0..efb24672b7d9 100644
>> --- a/drivers/cdx/cdx.c
>> +++ b/drivers/cdx/cdx.c
>> @@ -182,6 +182,38 @@ cdx_match_id(const struct cdx_device_id *ids, struct cdx_device *dev)
>> return NULL;
>> }
>>
>> +int cdx_set_master(struct cdx_device *cdx_dev)
>> +{
>> + struct cdx_controller *cdx = cdx_dev->cdx;
>> + struct cdx_device_config dev_config;
>> + int ret;
>> +
>> + dev_config.type = CDX_DEV_BUS_MASTER_CONF;
>> + dev_config.bme = true;
>
> What is "bme"?
This is bus master enable. I will add a comment on the structure definition.
>
> And are you sure that the config can be on the stack?
Yes, as this information is used by the controller and a new created
command is sent to the Firmware/Hardware.
>
>> + ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num,
>> + cdx_dev->dev_num, &dev_config);
>
> Will dev_configure always be there?
This is expected to be there, but will add a check for further assurance.
>
>> + if (ret)
>> + dev_err(&cdx_dev->dev, "device master enable failed\n");
>
> Why error out here, shouldn't the call have printed something if it
> wanted to?
Agree, will remove
>
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(cdx_set_master);
>> +
>> +void cdx_clear_master(struct cdx_device *cdx_dev)
>> +{
>> + struct cdx_controller *cdx = cdx_dev->cdx;
>> + struct cdx_device_config dev_config;
>> + int ret;
>> +
>> + dev_config.type = CDX_DEV_BUS_MASTER_CONF;
>> + dev_config.bme = false;
>> + ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num,
>> + cdx_dev->dev_num, &dev_config);
>> + if (ret)
>> + dev_err(&cdx_dev->dev, "device master disable failed\n");
>
> Same question as above.
sure will remove
>
>> +}
>> +EXPORT_SYMBOL_GPL(cdx_clear_master);
>> +
>> /**
>> * cdx_bus_match - device to driver matching callback
>> * @dev: the cdx device to match against
>> diff --git a/drivers/cdx/controller/cdx_controller.c b/drivers/cdx/controller/cdx_controller.c
>> index dc52f95f8978..ba670f0427d3 100644
>> --- a/drivers/cdx/controller/cdx_controller.c
>> +++ b/drivers/cdx/controller/cdx_controller.c
>> @@ -55,6 +55,10 @@ static int cdx_configure_device(struct cdx_controller *cdx,
>> case CDX_DEV_RESET_CONF:
>> ret = cdx_mcdi_reset_device(cdx->priv, bus_num, dev_num);
>> break;
>> + case CDX_DEV_BUS_MASTER_CONF:
>> + ret = cdx_mcdi_bus_master_enable(cdx->priv, bus_num, dev_num,
>> + dev_config->bme);
>> + break;
>> default:
>> ret = -EINVAL;
>> }
>> diff --git a/drivers/cdx/controller/mcdi_functions.c b/drivers/cdx/controller/mcdi_functions.c
>> index 0158f26533dd..c4c00f376006 100644
>> --- a/drivers/cdx/controller/mcdi_functions.c
>> +++ b/drivers/cdx/controller/mcdi_functions.c
>> @@ -137,3 +137,60 @@ int cdx_mcdi_reset_device(struct cdx_mcdi *cdx, u8 bus_num, u8 dev_num)
>>
>> return ret;
>> }
>> +
>> +static int cdx_mcdi_ctrl_flag_get(struct cdx_mcdi *cdx, u8 bus_num,
>> + u8 dev_num, u32 *flags)
>> +{
>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_DEVICE_CONTROL_GET_IN_LEN);
>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_CDX_DEVICE_CONTROL_GET_OUT_LEN);
>
> How big are these buffers?
The defines are as follows in mc_cdx_pcol.h:
#define MC_CMD_CDX_DEVICE_CONTROL_GET_IN_LEN 8
#define MC_CMD_CDX_DEVICE_CONTROL_GET_OUT_LEN 4
where MCDI_DECLARE_BUF leads to creation of a variable of data structure:
/* A doubleword (i.e. 4 byte) datatype - little-endian in HW */
struct cdx_dword {
__le32 cdx_u32;
};
so inbuf is of 32 bytes (8 * 4) and outbuf is 16 bytes (4 * 4).
>
>> + size_t outlen;
>> + int ret;
>> +
>> + MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_GET_IN_BUS, bus_num);
>> + MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_GET_IN_DEVICE, dev_num);
>> + ret = cdx_mcdi_rpc(cdx, MC_CMD_CDX_DEVICE_CONTROL_GET, inbuf,
>> + sizeof(inbuf), outbuf, sizeof(outbuf), &outlen);
>> + if (ret)
>> + return ret;
>> +
>> + if (outlen != MC_CMD_CDX_DEVICE_CONTROL_GET_OUT_LEN)
>> + return -EIO;
>> +
>> + *flags = MCDI_DWORD(outbuf, CDX_DEVICE_CONTROL_GET_OUT_FLAGS);
>> +
>> + return 0;
>> +}
>> +
>> +static int cdx_mcdi_ctrl_flag_set(struct cdx_mcdi *cdx, u8 bus_num,
>> + u8 dev_num, bool enable, int lbn)
>> +{
>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_DEVICE_CONTROL_SET_IN_LEN);
>> + int ret, en;
>> + u32 flags;
>> +
>> + /*
>> + * Get flags and then set/reset BUS_MASTER_BIT according to
>> + * the input params.
>> + */
>> + ret = cdx_mcdi_ctrl_flag_get(cdx, bus_num, dev_num, &flags);
>> + if (ret)
>> + return ret;
>> +
>> + en = enable ? 1 : 0;
>
> if/then/else?
okay. will update
>
>
>> + flags = (flags & (u32)(~(BIT(lbn)))) | (en << lbn);
>> +
>> + MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_SET_IN_BUS, bus_num);
>> + MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_SET_IN_DEVICE, dev_num);
>> + MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_SET_IN_FLAGS, flags);
>> + ret = cdx_mcdi_rpc(cdx, MC_CMD_CDX_DEVICE_CONTROL_SET, inbuf,
>> + sizeof(inbuf), NULL, 0, NULL);
>> +
>> + return ret;
>> +}
>> +
>> +int cdx_mcdi_bus_master_enable(struct cdx_mcdi *cdx, u8 bus_num,
>> + u8 dev_num, bool enable)
>> +{
>> + return cdx_mcdi_ctrl_flag_set(cdx, bus_num, dev_num, enable,
>> + MC_CMD_CDX_DEVICE_CONTROL_SET_IN_BUS_MASTER_ENABLE_LBN);
>> +}
>> diff --git a/drivers/cdx/controller/mcdi_functions.h b/drivers/cdx/controller/mcdi_functions.h
>> index 7440ace5539a..a448d6581eb4 100644
>> --- a/drivers/cdx/controller/mcdi_functions.h
>> +++ b/drivers/cdx/controller/mcdi_functions.h
>> @@ -58,4 +58,17 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx,
>> int cdx_mcdi_reset_device(struct cdx_mcdi *cdx,
>> u8 bus_num, u8 dev_num);
>>
>> +/**
>> + * cdx_mcdi_bus_master_enable - Set/Reset bus mastering for cdx device
>> + * represented by bus_num:dev_num
>> + * @cdx: pointer to MCDI interface.
>> + * @bus_num: Bus number.
>> + * @dev_num: Device number.
>> + * @enable: Enable bus mastering if set, disable otherwise.
>> + *
>> + * Return: 0 on success, <0 on failure
>> + */
>> +int cdx_mcdi_bus_master_enable(struct cdx_mcdi *cdx, u8 bus_num,
>> + u8 dev_num, bool enable);
>> +
>> #endif /* CDX_MCDI_FUNCTIONS_H */
>> diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h
>> index bead71b7bc73..acb4e40bd20e 100644
>> --- a/include/linux/cdx/cdx_bus.h
>> +++ b/include/linux/cdx/cdx_bus.h
>> @@ -21,11 +21,13 @@
>> struct cdx_controller;
>>
>> enum {
>> + CDX_DEV_BUS_MASTER_CONF,
>> CDX_DEV_RESET_CONF,
>> };
>>
>> struct cdx_device_config {
>> u8 type;
>> + bool bme;
>
> What does "bme" mean?
This is bus master enable bit. Will add comment.
Thanks,
Nipun
>
> thanks,
>
> greg k-h
Powered by blists - more mailing lists