* [PATCH] net/ncsi: Add NCSI OEM command for FB Tiogapass
From: Vijay Khemka @ 2018-09-28 22:26 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <81718e3fd6c883917aca540c032a7065fd00e79a.camel@mendozajonas.com>
?On 9/26/18, 8:44 PM, "Samuel Mendoza-Jonas" <sam@mendozajonas.com> wrote:
> Hi Vijay,
> Having had a chance to take a closer look, there is probably room for
> both this patchset and Justin's potential changes to coexist; while
> Justin's is a more general solution for sending arbitrary commands, the
> approach of this patch is also useful for handling commands we want
> included in the configure process (such as get-mac-address).
Hi Sam,
I have created a more generic approach based patch, will send you after clean up. I agree we need both Justin patch as well as mine to handle OEM specific command. I am creating 2 patches one for generic OEM command and response handling and another is OEM vendor specific command handling. Please see my responses to your comments below.
Some comments below:
> ---
> net/ncsi/Kconfig | 3 ++
> net/ncsi/internal.h | 11 +++++--
> net/ncsi/ncsi-cmd.c | 24 +++++++++++++--
> net/ncsi/ncsi-manage.c | 68 ++++++++++++++++++++++++++++++++++++++++++
> net/ncsi/ncsi-pkt.h | 16 ++++++++++
> net/ncsi/ncsi-rsp.c | 33 +++++++++++++++++++-
> 6 files changed, 149 insertions(+), 6 deletions(-)
>
> diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
> index 08a8a6031fd7..b8bf89fea7c8 100644
> --- a/net/ncsi/Kconfig
> +++ b/net/ncsi/Kconfig
> @@ -10,3 +10,6 @@ config NET_NCSI
> support. Enable this only if your system connects to a network
> device via NCSI and the ethernet driver you're using supports
> the protocol explicitly.
> +config NCSI_OEM_CMD_GET_MAC
> + bool "Get NCSI OEM MAC Address"
> + depends on NET_NCSI
For the moment this isn't too bad but I wonder if in the future it would
be more flexible to have something like NCSI_OEM_CMD_MELLANOX etc, so we
could selectively enable a class of OEM commands based on vendor rather
than per-command.
Certainly, we can have like that and will be an addition to above config.
Above config is to enable retrieving and setting mac address from device
during channel configuration. And then we can have vendor selection like
MELLANOX, Broadcom etc. But I will rather check GV ID under this config
and see if there is available function for specific vendor. This can be revised
and made more generic based on vendor.
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 8055e3965cef..da17958e6a4b 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -68,6 +68,10 @@ enum {
> NCSI_MODE_MAX
> };
>
> +#define NCSI_OEM_MFR_MLX_ID 0x8119
> +#define NCSI_OEM_MLX_CMD_GET_MAC 0x1b00
> +#define NCSI_OEM_MLX_CMD_SET_AFFINITY 0x010700
I gather this is part of the OEM command but it would be good to describe
what these bits mean. Is this command documented anywhere by Mellanox?
I will add more description here, please see in next patch. Yes Mellanox
specification describes these commands.
> +
> struct ncsi_channel_version {
> u32 version; /* Supported BCD encoded NCSI version */
> u32 alpha2; /* Supported BCD encoded NCSI version */
> @@ -236,6 +240,7 @@ enum {
> ncsi_dev_state_probe_dp,
> ncsi_dev_state_config_sp = 0x0301,
> ncsi_dev_state_config_cis,
> + ncsi_dev_state_config_oem_gma,
> ncsi_dev_state_config_clear_vids,
> ncsi_dev_state_config_svf,
> ncsi_dev_state_config_ev,
> @@ -301,9 +306,9 @@ struct ncsi_cmd_arg {
> unsigned short payload; /* Command packet payload length */
> unsigned int req_flags; /* NCSI request properties */
> union {
> - unsigned char bytes[16]; /* Command packet specific data */
> - unsigned short words[8];
> - unsigned int dwords[4];
> + unsigned char bytes[64]; /* Command packet specific data */
> + unsigned short words[32];
> + unsigned int dwords[16];
> };
> };
>
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index 7567ca63aae2..3205e22c1734 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -211,6 +211,25 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> return 0;
> }
>
> +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> + struct ncsi_cmd_arg *nca)
> +{
> + struct ncsi_cmd_oem_pkt *cmd;
> + unsigned int len;
> +
> + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> + if (nca->payload < 26)
> + len += 26;
This will have already happened in ncsi_alloc_command(), is this check
needed?
Yes it is needed to find length for skbuff data to initialize.
ncsi_alloc_command() does the same and allocate skbuff.
> + else
> + len += nca->payload;
> +
> + cmd = skb_put_zero(skb, len);
> + memcpy(cmd->data, nca->bytes, nca->payload);
> + ncsi_cmd_build_header(&cmd->cmd.common, nca);
> +
> + return 0;
> +}
> +
> static struct ncsi_cmd_handler {
> unsigned char type;
> int payload;
> @@ -244,7 +263,7 @@ static struct ncsi_cmd_handler {
> { NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
> { NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
> { NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_OEM, 0, NULL },
> + { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
> { NCSI_PKT_CMD_PLDM, 0, NULL },
> { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> };
> @@ -317,7 +336,8 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> }
>
> /* Get packet payload length and allocate the request */
> - nca->payload = nch->payload;
> + if (nch->payload >= 0)
> + nca->payload = nch->payload;
I think with this there is a chance of nca->payload being uninitialised
and then used in ncsi_alloc_command(). Can you describe what the aim here
is?
I will add more description here.
> nr = ncsi_alloc_command(nca);
> if (!nr)
> return -ENOMEM;
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 091284760d21..3b2b86560cc8 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -635,6 +635,58 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> +/* NCSI Facebook OEM APIs */
Are these Facebook OEM commands or Mellanox OEM commands?
Will be taken care in next patch
> +static void get_mac_address_oem_mlx(struct ncsi_dev_priv *ndp)
> +{
> + struct ncsi_cmd_arg nca;
> + int ret = 0;
> +
> + memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
> + nca.ndp = ndp;
> + nca.channel = ndp->active_channel->id;
> + nca.package = ndp->active_package->id;
> + nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> + nca.type = NCSI_PKT_CMD_OEM;
> + nca.payload = 8;
> +
> + nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
> + nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_GET_MAC);
> +
> + ret = ncsi_xmit_cmd(&nca);
> + if (ret)
> + netdev_err(ndp->ndev.dev,
> + "NCSI: Failed to transmit cmd 0x%x during probe\n",
> + nca.type);
> +}
> +
> +static void set_mac_affinity_mlx(struct ncsi_dev_priv *ndp)
> +{
> + struct ncsi_cmd_arg nca;
> + int ret = 0;
> +
> + memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
Ah I see we initialise nca, and thus payload here - the path between
these two points in the driver isn't super obvious (not this patch's
fault :) ), it might be better to explicitly mention/handle this where we
use payload later on.
Yes, I agree, will add more details.
> + nca.ndp = ndp;
> + nca.channel = ndp->active_channel->id;
> + nca.package = ndp->active_package->id;
These should probably be set in ncsi_configure_channel (eg. see the CIS
state before these are called).
These functions are being called from ncsi_configure_channel() only. We
can either initialize nca there in ncsi_configure_channel() function and pass
nca as function parameter or initialize nca inside function and populate
required field.
> + nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> + nca.type = NCSI_PKT_CMD_OEM;
> + nca.payload = 60;
> +
> + nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
> + nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_SET_AFFINITY);
> +
> + memcpy(&(nca.bytes[8]), ndp->ndev.dev->dev_addr, ETH_ALEN);
> + nca.bytes[14] = 0x09;
> +
> + ret = ncsi_xmit_cmd(&nca);
> + if (ret)
> + netdev_err(ndp->ndev.dev,
> + "NCSI: Failed to transmit cmd 0x%x during probe\n",
> + nca.type);
> +}
> +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
> +
> static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> {
> struct ncsi_dev *nd = &ndp->ndev;
> @@ -685,6 +737,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> goto error;
> }
>
> +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> + /* Check Manufacture id if it is Mellanox then
> + * get and set mac address. To Do: Add code for
> + * other types of card if required
> + */
> + if (nc->version.mf_id == NCSI_OEM_MFR_MLX_ID)
> + nd->state = ncsi_dev_state_config_oem_gma;
> + else
> + nd->state = ncsi_dev_state_config_clear_vids;
> + break;
> + case ncsi_dev_state_config_oem_gma:
> + ndp->pending_req_num = 2;
> + get_mac_address_oem_mlx(ndp);
> + msleep(500);
Is this msleep() required?
It is required to make sure previous command completed.
Will try to handle it differently
> + set_mac_affinity_mlx(ndp);
Since this *sets* the MAC address, should we name the state and config
option more accurately? How does this OEM command interact with the NCSI
set-mac-address command?
Does this command depend on the previous get_mac_address_oem_mlx()
command succeeding?
It is independent of Set_mac_address. Yes it depends on succession
of get_mac_address_oem_mlx() that's why msleep was put there.
> +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
> nd->state = ncsi_dev_state_config_clear_vids;
> break;
> case ncsi_dev_state_config_clear_vids:
> diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> index 91b4b66438df..0653a893eb12 100644
> --- a/net/ncsi/ncsi-pkt.h
> +++ b/net/ncsi/ncsi-pkt.h
> @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
> unsigned char pad[22];
> };
>
> +/* Oem Request Command */
In general, s/Oem/OEM
Sure, will be done.
> +struct ncsi_cmd_oem_pkt {
> + struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> + unsigned char data[64]; /* OEM Payload Data */
> + __be32 checksum; /* Checksum */
> +};
> +
> +/* Oem Response Packet */
> +struct ncsi_rsp_oem_pkt {
> + struct ncsi_rsp_pkt_hdr rsp; /* Command header */
> + __be32 mfr_id; /* Manufacture ID */
> + __be32 oem_cmd; /* oem command */
> + unsigned char data[32]; /* Payload data */
> + __be32 checksum; /* Checksum */
> +};
> +
> /* Get Link Status */
> struct ncsi_rsp_gls_pkt {
> struct ncsi_rsp_pkt_hdr rsp; /* Response header */
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 930c1d3796f0..3b94c96b9c7f 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -596,6 +596,37 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
> return 0;
> }
>
> +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> +{
> + struct ncsi_rsp_oem_pkt *rsp;
> + struct ncsi_dev_priv *ndp = nr->ndp;
> + struct net_device *ndev = ndp->ndev.dev;
> + int ret = 0;
> + unsigned int oem_cmd, mfr_id;
> + const struct net_device_ops *ops = ndev->netdev_ops;
> + struct sockaddr saddr;
> +
> + /* Get the response header */
> + rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> +
> + oem_cmd = ntohl(rsp->oem_cmd);
> + mfr_id = ntohl(rsp->mfr_id);
> +
> + /* Check for Mellanox manufacturer id */
> + if (mfr_id != NCSI_OEM_MFR_MLX_ID)
> + return 0;
> +
> + if (oem_cmd == NCSI_OEM_MLX_CMD_GET_MAC) {
> + saddr.sa_family = ndev->type;
> + ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> + memcpy(saddr.sa_data, &(rsp->data[4]), ETH_ALEN);
> + ret = ops->ndo_set_mac_address(ndev, &saddr);
> + if (ret < 0)
> + netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
> + }
> + return ret;
> +}
> +
> static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
> {
> struct ncsi_rsp_gvi_pkt *rsp;
> @@ -932,7 +963,7 @@ static struct ncsi_rsp_handler {
> { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> - { NCSI_PKT_RSP_OEM, 0, NULL },
> + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> { NCSI_PKT_RSP_PLDM, 0, NULL },
> { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> };
^ permalink raw reply
* [PATCH net v2] net/ncsi: Extend NC-SI Netlink interface to allow user space to send NC-SI command
From: Vijay Khemka @ 2018-09-28 23:22 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <58fff61ffd8d4038b59f32b696e76bbc@AUSX13MPS306.AMER.DELL.COM>
?>On 9/28/18, 11:16 AM, "Justin.Lee1 at Dell.com" <Justin.Lee1@Dell.com> wrote:
> The new command (NCSI_CMD_SEND_CMD) is added to allow user space application
> to send NC-SI command to the network card.
> Also, add a new attribute (NCSI_ATTR_DATA) for transferring request and response.
>
> The work flow is as below.
>
> Request:
> User space application -> Netlink interface (msg)
-> new Netlink handler - ncsi_send_cmd_nl()
-> ncsi_xmit_cmd()
> Response:
> Response received - ncsi_rcv_rsp() -> internal response handler - ncsi_rsp_handler_xxx()
-> ncsi_rsp_handler_netlink()
-> ncsi_send_netlink_rsp ()
-> Netlink interface (msg)
-> user space application
> Command timeout - ncsi_request_timeout() -> ncsi_send_netlink_timeout ()
-> Netlink interface (msg with zero data length)
-> user space application
> Error:
> Error detected -> ncsi_send_netlink_err () -> Netlink interface (err msg)
-> user space application
I will request to keep 2 patch, one for OEM handler and other one for netlink user space handling.
^ permalink raw reply
* [PATCH v2] net/ncsi: Add NCSI OEM command support
From: Vijay Khemka @ 2018-09-29 1:06 UTC (permalink / raw)
To: linux-aspeed
This patch adds OEM commands and response handling. It also defines OEM
command and response structure as per NCSI specification along with its
handlers.
ncsi_cmd_handler_oem: This is a generic command request handler for OEM
commands
ncsi_rsp_handler_oem: This is a generic response handler for OEM commands
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
net/ncsi/internal.h | 4 ++++
net/ncsi/ncsi-cmd.c | 31 ++++++++++++++++++++++++++++---
net/ncsi/ncsi-pkt.h | 16 ++++++++++++++++
net/ncsi/ncsi-rsp.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 91 insertions(+), 4 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 8055e3965cef..c16cb7223064 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -68,6 +68,10 @@ enum {
NCSI_MODE_MAX
};
+/* OEM Vendor Manufacture ID */
+#define NCSI_OEM_MFR_MLX_ID 0x8119
+#define NCSI_OEM_MFR_BCM_ID 0x113d
+
struct ncsi_channel_version {
u32 version; /* Supported BCD encoded NCSI version */
u32 alpha2; /* Supported BCD encoded NCSI version */
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index 7567ca63aae2..2f98533eba46 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
return 0;
}
+static int ncsi_cmd_handler_oem(struct sk_buff *skb,
+ struct ncsi_cmd_arg *nca)
+{
+ struct ncsi_cmd_oem_pkt *cmd;
+ unsigned int len;
+
+ len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
+ if (nca->payload < 26)
+ len += 26;
+ else
+ len += nca->payload;
+
+ cmd = skb_put_zero(skb, len);
+ cmd->mfr_id = nca->dwords[0];
+ memcpy(cmd->data, &nca->dwords[1], nca->payload - 4);
+ ncsi_cmd_build_header(&cmd->cmd.common, nca);
+
+ return 0;
+}
+
static struct ncsi_cmd_handler {
unsigned char type;
int payload;
@@ -244,7 +264,7 @@ static struct ncsi_cmd_handler {
{ NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
{ NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
{ NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
- { NCSI_PKT_CMD_OEM, 0, NULL },
+ { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
{ NCSI_PKT_CMD_PLDM, 0, NULL },
{ NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
};
@@ -316,8 +336,13 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
return -ENOENT;
}
- /* Get packet payload length and allocate the request */
- nca->payload = nch->payload;
+ /* Get packet payload length and allocate the request
+ * It is expected that if length set as negative in
+ * handler structure means caller is initializing it
+ * and setting length in nca before calling xmit function
+ */
+ if (nch->payload >= 0)
+ nca->payload = nch->payload;
nr = ncsi_alloc_command(nca);
if (!nr)
return -ENOMEM;
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index 91b4b66438df..1f338386810d 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h
@@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
unsigned char pad[22];
};
+/* OEM Request Command as per NCSI Specification */
+struct ncsi_cmd_oem_pkt {
+ struct ncsi_cmd_pkt_hdr cmd; /* Command header */
+ __be32 mfr_id; /* Manufacture ID */
+ unsigned char data[64]; /* OEM Payload Data */
+ __be32 checksum; /* Checksum */
+};
+
+/* OEM Response Packet as per NCSI Specification */
+struct ncsi_rsp_oem_pkt {
+ struct ncsi_rsp_pkt_hdr rsp; /* Command header */
+ __be32 mfr_id; /* Manufacture ID */
+ unsigned char data[64]; /* Payload data */
+ __be32 checksum; /* Checksum */
+};
+
/* Get Link Status */
struct ncsi_rsp_gls_pkt {
struct ncsi_rsp_pkt_hdr rsp; /* Response header */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 930c1d3796f0..22664ebdc93a 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -596,6 +596,48 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
return 0;
}
+static struct ncsi_rsp_oem_handler {
+ unsigned int mfr_id;
+ int (*handler)(struct ncsi_request *nr);
+} ncsi_rsp_oem_handlers[] = {
+ { NCSI_OEM_MFR_MLX_ID, NULL },
+ { NCSI_OEM_MFR_BCM_ID, NULL }
+};
+
+
+/* Response handler for OEM command */
+static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
+{
+ struct ncsi_rsp_oem_pkt *rsp;
+ struct ncsi_rsp_oem_handler *nrh = NULL;
+ unsigned int mfr_id, i;
+
+ /* Get the response header */
+ rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+ mfr_id = ntohl(rsp->mfr_id);
+
+ /* Check for manufacturer id and Find the handler */
+ for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
+ if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
+ if (ncsi_rsp_oem_handlers[i].handler)
+ nrh = &ncsi_rsp_oem_handlers[i];
+ else
+ nrh = NULL;
+
+ break;
+ }
+ }
+
+ if (!nrh) {
+ netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM packet with MFR-ID (0x%x)\n",
+ mfr_id);
+ return -ENOENT;
+ }
+
+ /* Process the packet */
+ return nrh->handler(nr);
+}
+
static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
{
struct ncsi_rsp_gvi_pkt *rsp;
@@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
{ NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
{ NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
{ NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
- { NCSI_PKT_RSP_OEM, 0, NULL },
+ { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
{ NCSI_PKT_RSP_PLDM, 0, NULL },
{ NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
};
--
2.17.1
^ permalink raw reply related
* [PATCH v2] net/ncsi: Add NCSI OEM command support
From: Vijay Khemka @ 2018-09-29 1:20 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180929010602.1025909-1-vijaykhemka@fb.com>
?> On 9/28/18, 6:07 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
> This patch adds OEM commands and response handling. It also defines OEM
> command and response structure as per NCSI specification along with its
> handlers.
>
> ncsi_cmd_handler_oem: This is a generic command request handler for OEM
> commands
> ncsi_rsp_handler_oem: This is a generic response handler for OEM commands
This is a generic patch for OEM command handling, There will be another patch
following this to handle specific OEM commands for each vendor. Currently I have
defined 2 vendor/manufacturer id below in internal.h, more can be added here for
other vendors. I have not defined ncsi_rsp_oem_handler in this patch as they are
NULL, but there will be a defined handlers for each vendor in next patch.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
net/ncsi/internal.h | 4 ++++
net/ncsi/ncsi-cmd.c | 31 ++++++++++++++++++++++++++++---
net/ncsi/ncsi-pkt.h | 16 ++++++++++++++++
net/ncsi/ncsi-rsp.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 91 insertions(+), 4 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 8055e3965cef..c16cb7223064 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -68,6 +68,10 @@ enum {
NCSI_MODE_MAX
};
+/* OEM Vendor Manufacture ID */
+#define NCSI_OEM_MFR_MLX_ID 0x8119
+#define NCSI_OEM_MFR_BCM_ID 0x113d
+
struct ncsi_channel_version {
u32 version; /* Supported BCD encoded NCSI version */
u32 alpha2; /* Supported BCD encoded NCSI version */
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index 7567ca63aae2..2f98533eba46 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
return 0;
}
+static int ncsi_cmd_handler_oem(struct sk_buff *skb,
+ struct ncsi_cmd_arg *nca)
+{
+ struct ncsi_cmd_oem_pkt *cmd;
+ unsigned int len;
+
+ len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
+ if (nca->payload < 26)
+ len += 26;
+ else
+ len += nca->payload;
+
+ cmd = skb_put_zero(skb, len);
+ cmd->mfr_id = nca->dwords[0];
+ memcpy(cmd->data, &nca->dwords[1], nca->payload - 4);
+ ncsi_cmd_build_header(&cmd->cmd.common, nca);
+
+ return 0;
+}
+
static struct ncsi_cmd_handler {
unsigned char type;
int payload;
@@ -244,7 +264,7 @@ static struct ncsi_cmd_handler {
{ NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
{ NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
{ NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
- { NCSI_PKT_CMD_OEM, 0, NULL },
+ { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
{ NCSI_PKT_CMD_PLDM, 0, NULL },
{ NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
};
@@ -316,8 +336,13 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
return -ENOENT;
}
- /* Get packet payload length and allocate the request */
- nca->payload = nch->payload;
+ /* Get packet payload length and allocate the request
+ * It is expected that if length set as negative in
+ * handler structure means caller is initializing it
+ * and setting length in nca before calling xmit function
+ */
+ if (nch->payload >= 0)
+ nca->payload = nch->payload;
nr = ncsi_alloc_command(nca);
if (!nr)
return -ENOMEM;
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index 91b4b66438df..1f338386810d 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h
@@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
unsigned char pad[22];
};
+/* OEM Request Command as per NCSI Specification */
+struct ncsi_cmd_oem_pkt {
+ struct ncsi_cmd_pkt_hdr cmd; /* Command header */
+ __be32 mfr_id; /* Manufacture ID */
+ unsigned char data[64]; /* OEM Payload Data */
+ __be32 checksum; /* Checksum */
+};
+
+/* OEM Response Packet as per NCSI Specification */
+struct ncsi_rsp_oem_pkt {
+ struct ncsi_rsp_pkt_hdr rsp; /* Command header */
+ __be32 mfr_id; /* Manufacture ID */
+ unsigned char data[64]; /* Payload data */
+ __be32 checksum; /* Checksum */
+};
+
/* Get Link Status */
struct ncsi_rsp_gls_pkt {
struct ncsi_rsp_pkt_hdr rsp; /* Response header */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 930c1d3796f0..22664ebdc93a 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -596,6 +596,48 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
return 0;
}
+static struct ncsi_rsp_oem_handler {
+ unsigned int mfr_id;
+ int (*handler)(struct ncsi_request *nr);
+} ncsi_rsp_oem_handlers[] = {
+ { NCSI_OEM_MFR_MLX_ID, NULL },
+ { NCSI_OEM_MFR_BCM_ID, NULL }
+};
+
+
+/* Response handler for OEM command */
+static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
+{
+ struct ncsi_rsp_oem_pkt *rsp;
+ struct ncsi_rsp_oem_handler *nrh = NULL;
+ unsigned int mfr_id, i;
+
+ /* Get the response header */
+ rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+ mfr_id = ntohl(rsp->mfr_id);
+
+ /* Check for manufacturer id and Find the handler */
+ for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
+ if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
+ if (ncsi_rsp_oem_handlers[i].handler)
+ nrh = &ncsi_rsp_oem_handlers[i];
+ else
+ nrh = NULL;
+
+ break;
+ }
+ }
+
+ if (!nrh) {
+ netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM packet with MFR-ID (0x%x)\n",
+ mfr_id);
+ return -ENOENT;
+ }
+
+ /* Process the packet */
+ return nrh->handler(nr);
+}
+
static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
{
struct ncsi_rsp_gvi_pkt *rsp;
@@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
{ NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
{ NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
{ NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
- { NCSI_PKT_RSP_OEM, 0, NULL },
+ { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
{ NCSI_PKT_RSP_PLDM, 0, NULL },
{ NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
};
--
2.17.1
^ permalink raw reply
* [PATCH] usb: gadget: fix spelling mistakeis "[En]queing" -> "[En]queuing"
From: Colin King @ 2018-09-29 11:43 UTC (permalink / raw)
To: linux-aspeed
From: Colin Ian King <colin.king@canonical.com>
Trivial fix to spelling mistakes in debug warning messages
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 2 +-
drivers/usb/gadget/udc/udc-xilinx.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 5939eb1e97f2..e9ee2b72af19 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -353,7 +353,7 @@ static int ast_vhub_epn_queue(struct usb_ep* u_ep, struct usb_request *u_req,
/* Endpoint enabled ? */
if (!ep->epn.enabled || !u_ep->desc || !ep->dev || !ep->d_idx ||
!ep->dev->enabled || ep->dev->suspended) {
- EPDBG(ep,"Enqueing request on wrong or disabled EP\n");
+ EPDBG(ep, "Enqueuing request on wrong or disabled EP\n");
return -ESHUTDOWN;
}
diff --git a/drivers/usb/gadget/udc/udc-xilinx.c b/drivers/usb/gadget/udc/udc-xilinx.c
index 6407e433bc78..b1f4104d1283 100644
--- a/drivers/usb/gadget/udc/udc-xilinx.c
+++ b/drivers/usb/gadget/udc/udc-xilinx.c
@@ -1078,7 +1078,7 @@ static int xudc_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
unsigned long flags;
if (!ep->desc) {
- dev_dbg(udc->dev, "%s:queing request to disabled %s\n",
+ dev_dbg(udc->dev, "%s: queuing request to disabled %s\n",
__func__, ep->name);
return -ESHUTDOWN;
}
--
2.17.1
^ permalink raw reply related
* [PATCH v3 1/2] dt-bindings: media: Add Aspeed Video Engine binding documentation
From: Joel Stanley @ 2018-10-01 13:08 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1537903629-14003-2-git-send-email-eajames@linux.ibm.com>
On Tue, 25 Sep 2018 at 21:27, Eddie James <eajames@linux.ibm.com> wrote:
>
> Document the bindings.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
> .../devicetree/bindings/media/aspeed-video.txt | 26 ++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/aspeed-video.txt
>
> diff --git a/Documentation/devicetree/bindings/media/aspeed-video.txt b/Documentation/devicetree/bindings/media/aspeed-video.txt
> new file mode 100644
> index 0000000..f1af528
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/aspeed-video.txt
> @@ -0,0 +1,26 @@
> +* Device tree bindings for Aspeed Video Engine
> +
> +The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs can
> +capture and compress video data from digital or analog sources.
> +
> +Required properties:
> + - compatible: "aspeed,ast2400-video-engine" or
> + "aspeed,ast2500-video-engine"
> + - reg: contains the offset and length of the VE memory region
> + - clocks: clock specifiers for the syscon clocks associated with
> + the VE (ordering must match the clock-names property)
> + - clock-names: "vclk" and "eclk"
> + - resets: reset specifier for the syscon reset associaated with
associated
> + the VE
> + - interrupts: the interrupt associated with the VE on this platform
> +
> +Example:
> +
> +video-engine at 1e700000 {
> + compatible = "aspeed,ast2500-video-engine";
> + reg = <0x1e700000 0x20000>;
> + clocks = <&syscon ASPEED_CLK_GATE_VCLK>, <&syscon ASPEED_CLK_GATE_ECLK>;
> + clock-names = "vclk", "eclk";
Did you end up sending the clock patches out?
Cheers,
Joel
> + resets = <&syscon ASPEED_RESET_VIDEO>;
> + interrupts = <7>;
> +};
> --
> 1.8.3.1
>
^ permalink raw reply
* [PATCH v3 1/2] dt-bindings: media: Add Aspeed Video Engine binding documentation
From: Eddie James @ 2018-10-01 15:20 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACPK8Xd0MhrFQqiM=u-Rv5u7RJRo5R-pAejH4dmeTrYSWE0AZA@mail.gmail.com>
On 10/01/2018 08:08 AM, Joel Stanley wrote:
> On Tue, 25 Sep 2018 at 21:27, Eddie James <eajames@linux.ibm.com> wrote:
>> Document the bindings.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>> .../devicetree/bindings/media/aspeed-video.txt | 26 ++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/media/aspeed-video.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/aspeed-video.txt b/Documentation/devicetree/bindings/media/aspeed-video.txt
>> new file mode 100644
>> index 0000000..f1af528
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/aspeed-video.txt
>> @@ -0,0 +1,26 @@
>> +* Device tree bindings for Aspeed Video Engine
>> +
>> +The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs can
>> +capture and compress video data from digital or analog sources.
>> +
>> +Required properties:
>> + - compatible: "aspeed,ast2400-video-engine" or
>> + "aspeed,ast2500-video-engine"
>> + - reg: contains the offset and length of the VE memory region
>> + - clocks: clock specifiers for the syscon clocks associated with
>> + the VE (ordering must match the clock-names property)
>> + - clock-names: "vclk" and "eclk"
>> + - resets: reset specifier for the syscon reset associaated with
> associated
Good catch, thanks.
>
>> + the VE
>> + - interrupts: the interrupt associated with the VE on this platform
>> +
>> +Example:
>> +
>> +video-engine at 1e700000 {
>> + compatible = "aspeed,ast2500-video-engine";
>> + reg = <0x1e700000 0x20000>;
>> + clocks = <&syscon ASPEED_CLK_GATE_VCLK>, <&syscon ASPEED_CLK_GATE_ECLK>;
>> + clock-names = "vclk", "eclk";
> Did you end up sending the clock patches out?
Yes,
https://lore.kernel.org/patchwork/patch/978979/
https://lore.kernel.org/patchwork/patch/978976/
Do I need to send them as a separate series?
Thanks,
Eddie
>
> Cheers,
>
> Joel
>
>> + resets = <&syscon ASPEED_RESET_VIDEO>;
>> + interrupts = <7>;
>> +};
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* [PATCH i2c-next v4 0/3] i2c: aspeed: Add bus idle waiting logic for multi-master use cases
From: Jae Hyun Yoo @ 2018-10-01 20:27 UTC (permalink / raw)
To: linux-aspeed
In multi-master environment, this driver's master cannot know
exactly when peer master sends data to this driver's slave so a
case can be happened that this master tries to send data through
the master_xfer function but slave data from peer master is still
being processed by this driver.
To prevent state corruption in the case, this patch adds checking
code if any slave operation is ongoing and it waits up to the
timeout duration before starting a master_xfer operation.
Please review this patch set.
Thanks,
-Jae
Changes since v3:
- Changed the property name to 'timeout' and made it use the
default setting in i2c-core when not specified.
Changes since v2:
- Changed the property name to 'aspeed,timeout' and made it to
update the adapter's timeout configuration.
Changes since v1:
- Changed define names of timeout related.
Jae Hyun Yoo (3):
dt-bindings: i2c: aspeed: Add 'timeout' property as an optional
property
i2c: aspeed: Add 'timeout' DT property reading code
i2c: aspeed: Add bus idle waiting logic for multi-master use cases
.../devicetree/bindings/i2c/i2c-aspeed.txt | 3 +
drivers/i2c/busses/i2c-aspeed.c | 70 ++++++++++++++-----
2 files changed, 57 insertions(+), 16 deletions(-)
--
2.19.0
^ permalink raw reply
* [PATCH i2c-next v4 1/3] dt-bindings: i2c: aspeed: Add 'timeout' property as an optional property
From: Jae Hyun Yoo @ 2018-10-01 20:27 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181001202748.8030-1-jae.hyun.yoo@linux.intel.com>
This commit adds 'timeout' property as an optional property which
can be used for setting 'timeout' value of 'struct i2c_adapter'.
With this patch, the bus timeout value can be set through this
DT property from the probing time of the module. Still the bus
timeout value can be set by an I2C_TIMEOUT ioctl on cdev at
runtime too.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
Documentation/devicetree/bindings/i2c/i2c-aspeed.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
index 8fbd8633a387..1789502818c2 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -17,6 +17,9 @@ Optional Properties:
specified
- multi-master : states that there is another master active on this bus.
+- timeout : I2C bus timeout in milliseconds defaults to 1 second when not
+ specified.
+
Example:
i2c {
--
2.19.0
^ permalink raw reply related
* [PATCH i2c-next v4 2/3] i2c: aspeed: Add 'timeout' DT property reading code
From: Jae Hyun Yoo @ 2018-10-01 20:27 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181001202748.8030-1-jae.hyun.yoo@linux.intel.com>
This commit adds reading code of the 'timeout' DT property to set
bus timeout value in adapter configuration. This value still
can be configured through an I2C_TIMEOUT ioctl on cdev too.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
drivers/i2c/busses/i2c-aspeed.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 8dc9161ced38..6d31f54a6653 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -885,6 +885,7 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
struct clk *parent_clk;
struct resource *res;
int irq, ret;
+ u32 timeout_ms;
bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
if (!bus)
@@ -918,6 +919,11 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
bus->bus_frequency = 100000;
}
+ ret = of_property_read_u32(pdev->dev.of_node, "timeout",
+ &timeout_ms);
+ if (ret)
+ timeout_ms = 0; /* then adap.timeout will be set by i2c-core */
+
match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node);
if (!match)
bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
@@ -930,7 +936,7 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
init_completion(&bus->cmd_complete);
bus->adap.owner = THIS_MODULE;
bus->adap.retries = 0;
- bus->adap.timeout = 5 * HZ;
+ bus->adap.timeout = timeout_ms ? msecs_to_jiffies(timeout_ms) : 0;
bus->adap.algo = &aspeed_i2c_algo;
bus->adap.dev.parent = &pdev->dev;
bus->adap.dev.of_node = pdev->dev.of_node;
--
2.19.0
^ permalink raw reply related
* [PATCH i2c-next v4 3/3] i2c: aspeed: Add bus idle waiting logic for multi-master use cases
From: Jae Hyun Yoo @ 2018-10-01 20:27 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181001202748.8030-1-jae.hyun.yoo@linux.intel.com>
In multi-master environment, this driver's master cannot know
exactly when peer master sends data to this driver's slave so a
case can be happened that this master tries to send data through
the master_xfer function but slave data from peer master is still
being processed by this driver.
To prevent state corruption in the case, this patch adds checking
code if any slave operation is ongoing and it waits up to the
timeout duration before starting a master_xfer operation. Since
the BUSY bit in the I2CD14 register doesn't cover the whole slave
sequence, it uses 'Transfer Mode State Machine' bit fields to
make up the problem of the BUSY bit.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
drivers/i2c/busses/i2c-aspeed.c | 62 +++++++++++++++++++++++++--------
1 file changed, 47 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 6d31f54a6653..edb1944c867b 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -12,6 +12,7 @@
#include <linux/clk.h>
#include <linux/completion.h>
+#include <linux/delay.h>
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/i2c.h>
@@ -99,6 +100,7 @@
ASPEED_I2CD_INTR_TX_ACK)
/* 0x14 : I2CD Command/Status Register */
+#define ASPEED_I2CD_XFER_MODE_STS_MASK GENMASK(22, 19)
#define ASPEED_I2CD_SCL_LINE_STS BIT(18)
#define ASPEED_I2CD_SDA_LINE_STS BIT(17)
#define ASPEED_I2CD_BUS_BUSY_STS BIT(16)
@@ -115,6 +117,9 @@
/* 0x18 : I2CD Slave Device Address Register */
#define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
+/* Busy checking */
+#define ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US (10 * 1000)
+
enum aspeed_i2c_master_state {
ASPEED_I2C_MASTER_INACTIVE,
ASPEED_I2C_MASTER_START,
@@ -156,6 +161,8 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ /* Multi-master */
+ bool multi_master;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -596,27 +603,50 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
+static int aspeed_i2c_check_bus_busy(struct aspeed_i2c_bus *bus)
+{
+ u32 status_check_mask = ASPEED_I2CD_BUS_BUSY_STS;
+ ktime_t timeout;
+
+ if (bus->multi_master) {
+ might_sleep();
+ timeout = ktime_add_ms(ktime_get(),
+ jiffies_to_msecs(bus->adap.timeout));
+ /*
+ * ASPEED_I2CD_XFER_MODE_STS_MASK is marked as
+ * 'for debugging purpose only' in datasheet but ASPEED
+ * confirmed that this reflects real information and good to be
+ * used in practical code. It will be used only in multi-master
+ * use cases.
+ */
+ status_check_mask |= ASPEED_I2CD_XFER_MODE_STS_MASK;
+ }
+
+ for (;;) {
+ if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
+ status_check_mask))
+ return 0;
+ if (!bus->multi_master)
+ break;
+ if (ktime_compare(ktime_get(), timeout) > 0)
+ break;
+ usleep_range((ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US >> 2) + 1,
+ ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US);
+ }
+
+ return aspeed_i2c_recover_bus(bus);
+}
+
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
{
struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
unsigned long time_left, flags;
- int ret = 0;
- spin_lock_irqsave(&bus->lock, flags);
- bus->cmd_err = 0;
-
- /* If bus is busy, attempt recovery. We assume a single master
- * environment.
- */
- if (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_BUS_BUSY_STS) {
- spin_unlock_irqrestore(&bus->lock, flags);
- ret = aspeed_i2c_recover_bus(bus);
- if (ret)
- return ret;
- spin_lock_irqsave(&bus->lock, flags);
- }
+ if (aspeed_i2c_check_bus_busy(bus))
+ return -EAGAIN;
+ spin_lock_irqsave(&bus->lock, flags);
bus->cmd_err = 0;
bus->msgs = msgs;
bus->msgs_index = 0;
@@ -827,7 +857,9 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
if (ret < 0)
return ret;
- if (!of_property_read_bool(pdev->dev.of_node, "multi-master"))
+ if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
+ bus->multi_master = true;
+ else
fun_ctrl_reg |= ASPEED_I2CD_MULTI_MASTER_DIS;
/* Enable Master Mode */
--
2.19.0
^ permalink raw reply related
* [Potential Spoof] Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
From: Vijay Khemka @ 2018-10-01 22:45 UTC (permalink / raw)
To: linux-aspeed
?On 9/28/18, 6:21 PM, "Linux-aspeed on behalf of Vijay Khemka" <linux-aspeed-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of vijaykhemka@fb.com> wrote:
> On 9/28/18, 6:07 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
> This patch adds OEM commands and response handling. It also defines OEM
> command and response structure as per NCSI specification along with its
> handlers.
>
> ncsi_cmd_handler_oem: This is a generic command request handler for OEM
> commands
> ncsi_rsp_handler_oem: This is a generic response handler for OEM commands
This is a generic patch for OEM command handling, There will be another patch
following this to handle specific OEM commands for each vendor. Currently I have
defined 2 vendor/manufacturer id below in internal.h, more can be added here for
other vendors. I have not defined ncsi_rsp_oem_handler in this patch as they are
NULL, but there will be a defined handlers for each vendor in next patch.
Sam, Joel, Justin, please review this patch. I have 2 more patch coming for Broadcom and Mellanox.
^ permalink raw reply
* [PATCH v2] net/ncsi: Add NCSI OEM command support
From: Samuel Mendoza-Jonas @ 2018-10-02 1:20 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180929010602.1025909-1-vijaykhemka@fb.com>
On Fri, 2018-09-28 at 18:06 -0700, Vijay Khemka wrote:
> This patch adds OEM commands and response handling. It also defines OEM
> command and response structure as per NCSI specification along with its
> handlers.
>
> ncsi_cmd_handler_oem: This is a generic command request handler for OEM
> commands
> ncsi_rsp_handler_oem: This is a generic response handler for OEM commands
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
Hi Vijay - looks good to me, and should be a good common base for your
and Justin's changes.
Reviewed-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> ---
> net/ncsi/internal.h | 4 ++++
> net/ncsi/ncsi-cmd.c | 31 ++++++++++++++++++++++++++++---
> net/ncsi/ncsi-pkt.h | 16 ++++++++++++++++
> net/ncsi/ncsi-rsp.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 91 insertions(+), 4 deletions(-)
>
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 8055e3965cef..c16cb7223064 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -68,6 +68,10 @@ enum {
> NCSI_MODE_MAX
> };
>
> +/* OEM Vendor Manufacture ID */
> +#define NCSI_OEM_MFR_MLX_ID 0x8119
> +#define NCSI_OEM_MFR_BCM_ID 0x113d
> +
> struct ncsi_channel_version {
> u32 version; /* Supported BCD encoded NCSI version */
> u32 alpha2; /* Supported BCD encoded NCSI version */
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index 7567ca63aae2..2f98533eba46 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> return 0;
> }
>
> +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> + struct ncsi_cmd_arg *nca)
> +{
> + struct ncsi_cmd_oem_pkt *cmd;
> + unsigned int len;
> +
> + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> + if (nca->payload < 26)
> + len += 26;
> + else
> + len += nca->payload;
> +
> + cmd = skb_put_zero(skb, len);
> + cmd->mfr_id = nca->dwords[0];
> + memcpy(cmd->data, &nca->dwords[1], nca->payload - 4);
> + ncsi_cmd_build_header(&cmd->cmd.common, nca);
> +
> + return 0;
> +}
> +
> static struct ncsi_cmd_handler {
> unsigned char type;
> int payload;
> @@ -244,7 +264,7 @@ static struct ncsi_cmd_handler {
> { NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
> { NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
> { NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_OEM, 0, NULL },
> + { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
> { NCSI_PKT_CMD_PLDM, 0, NULL },
> { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> };
> @@ -316,8 +336,13 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> return -ENOENT;
> }
>
> - /* Get packet payload length and allocate the request */
> - nca->payload = nch->payload;
> + /* Get packet payload length and allocate the request
> + * It is expected that if length set as negative in
> + * handler structure means caller is initializing it
> + * and setting length in nca before calling xmit function
> + */
> + if (nch->payload >= 0)
> + nca->payload = nch->payload;
> nr = ncsi_alloc_command(nca);
> if (!nr)
> return -ENOMEM;
> diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> index 91b4b66438df..1f338386810d 100644
> --- a/net/ncsi/ncsi-pkt.h
> +++ b/net/ncsi/ncsi-pkt.h
> @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
> unsigned char pad[22];
> };
>
> +/* OEM Request Command as per NCSI Specification */
> +struct ncsi_cmd_oem_pkt {
> + struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> + __be32 mfr_id; /* Manufacture ID */
> + unsigned char data[64]; /* OEM Payload Data */
> + __be32 checksum; /* Checksum */
> +};
> +
> +/* OEM Response Packet as per NCSI Specification */
> +struct ncsi_rsp_oem_pkt {
> + struct ncsi_rsp_pkt_hdr rsp; /* Command header */
> + __be32 mfr_id; /* Manufacture ID */
> + unsigned char data[64]; /* Payload data */
> + __be32 checksum; /* Checksum */
> +};
> +
> /* Get Link Status */
> struct ncsi_rsp_gls_pkt {
> struct ncsi_rsp_pkt_hdr rsp; /* Response header */
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 930c1d3796f0..22664ebdc93a 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -596,6 +596,48 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
> return 0;
> }
>
> +static struct ncsi_rsp_oem_handler {
> + unsigned int mfr_id;
> + int (*handler)(struct ncsi_request *nr);
> +} ncsi_rsp_oem_handlers[] = {
> + { NCSI_OEM_MFR_MLX_ID, NULL },
> + { NCSI_OEM_MFR_BCM_ID, NULL }
> +};
> +
> +
> +/* Response handler for OEM command */
> +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> +{
> + struct ncsi_rsp_oem_pkt *rsp;
> + struct ncsi_rsp_oem_handler *nrh = NULL;
> + unsigned int mfr_id, i;
> +
> + /* Get the response header */
> + rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> + mfr_id = ntohl(rsp->mfr_id);
> +
> + /* Check for manufacturer id and Find the handler */
> + for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
> + if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
> + if (ncsi_rsp_oem_handlers[i].handler)
> + nrh = &ncsi_rsp_oem_handlers[i];
> + else
> + nrh = NULL;
> +
> + break;
> + }
> + }
> +
> + if (!nrh) {
> + netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM packet with MFR-ID (0x%x)\n",
> + mfr_id);
> + return -ENOENT;
> + }
> +
> + /* Process the packet */
> + return nrh->handler(nr);
> +}
> +
> static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
> {
> struct ncsi_rsp_gvi_pkt *rsp;
> @@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
> { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> - { NCSI_PKT_RSP_OEM, 0, NULL },
> + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> { NCSI_PKT_RSP_PLDM, 0, NULL },
> { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> };
^ permalink raw reply
* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
From: Tao Ren @ 2018-10-02 4:14 UTC (permalink / raw)
To: linux-aspeed
TIMER_INTR_MASK register (Base Address of Timer + 0x38) is not designed
for masking interrupts on ast2500 chips, and it's not even listed in
ast2400 datasheet, so it's not safe to access TIMER_INTR_MASK on aspeed
chips.
Similarly, TIMER_INTR_STATE register (Base Address of Timer + 0x34) is
not interrupt status register on ast2400 and ast2500 chips. Although
there is no side effect to reset the register in fttmr010_common_init(),
it's just misleading to do so.
Signed-off-by: Tao Ren <taoren@fb.com>
---
drivers/clocksource/timer-fttmr010.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index c020038ebfab..daf063c9842e 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -171,16 +171,17 @@ static int fttmr010_timer_set_oneshot(struct clock_event_device *evt)
/* Setup counter start from 0 or ~0 */
writel(0, fttmr010->base + TIMER1_COUNT);
- if (fttmr010->count_down)
+ if (fttmr010->count_down) {
writel(~0, fttmr010->base + TIMER1_LOAD);
- else
+ } else {
writel(0, fttmr010->base + TIMER1_LOAD);
- /* Enable interrupt */
- cr = readl(fttmr010->base + TIMER_INTR_MASK);
- cr &= ~(TIMER_1_INT_OVERFLOW | TIMER_1_INT_MATCH2);
- cr |= TIMER_1_INT_MATCH1;
- writel(cr, fttmr010->base + TIMER_INTR_MASK);
+ /* Enable interrupt */
+ cr = readl(fttmr010->base + TIMER_INTR_MASK);
+ cr &= ~(TIMER_1_INT_OVERFLOW | TIMER_1_INT_MATCH2);
+ cr |= TIMER_1_INT_MATCH1;
+ writel(cr, fttmr010->base + TIMER_INTR_MASK);
+ }
return 0;
}
@@ -287,13 +288,13 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
fttmr010->count_down = true;
} else {
fttmr010->t1_enable_val = TIMER_1_CR_ENABLE | TIMER_1_CR_INT;
- }
- /*
- * Reset the interrupt mask and status
- */
- writel(TIMER_INT_ALL_MASK, fttmr010->base + TIMER_INTR_MASK);
- writel(0, fttmr010->base + TIMER_INTR_STATE);
+ /*
+ * Reset the interrupt mask and status
+ */
+ writel(TIMER_INT_ALL_MASK, fttmr010->base + TIMER_INTR_MASK);
+ writel(0, fttmr010->base + TIMER_INTR_STATE);
+ }
/*
* Enable timer 1 count up, timer 2 count up, except on Aspeed,
--
2.17.1
^ permalink raw reply related
* [PATCH net v2] net/ncsi: Extend NC-SI Netlink interface to allow user space to send NC-SI command
From: Samuel Mendoza-Jonas @ 2018-10-02 5:11 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <58fff61ffd8d4038b59f32b696e76bbc@AUSX13MPS306.AMER.DELL.COM>
On Fri, 2018-09-28 at 18:15 +0000, Justin.Lee1 at Dell.com wrote:
> The new command (NCSI_CMD_SEND_CMD) is added to allow user space application
> to send NC-SI command to the network card.
> Also, add a new attribute (NCSI_ATTR_DATA) for transferring request and response.
>
> The work flow is as below.
>
> Request:
> User space application -> Netlink interface (msg)
> -> new Netlink handler - ncsi_send_cmd_nl()
> -> ncsi_xmit_cmd()
> Response:
> Response received - ncsi_rcv_rsp() -> internal response handler - ncsi_rsp_handler_xxx()
> -> ncsi_rsp_handler_netlink()
> -> ncsi_send_netlink_rsp ()
> -> Netlink interface (msg)
> -> user space application
> Command timeout - ncsi_request_timeout() -> ncsi_send_netlink_timeout ()
> -> Netlink interface (msg with zero data length)
> -> user space application
> Error:
> Error detected -> ncsi_send_netlink_err () -> Netlink interface (err msg)
> -> user space application
>
>
> Signed-off-by: Justin Lee <justin.lee1@dell.com>
Hi Justin,
This is looking pretty good, combined with Vijay's base patch the two
approaches should fit together nicely (
http://patchwork.ozlabs.org/patch/976510/).
A good merge order would probably be the above patch first, then this
patch and Vijay's further OEM patches based on top of that to reduce
conflicts.
Cheers,
Sam
>
> ---
> include/uapi/linux/ncsi.h | 3 +
> net/ncsi/internal.h | 12 ++-
> net/ncsi/ncsi-cmd.c | 47 ++++++++++-
> net/ncsi/ncsi-manage.c | 22 +++++
> net/ncsi/ncsi-netlink.c | 205 ++++++++++++++++++++++++++++++++++++++++++++++
> net/ncsi/ncsi-netlink.h | 12 +++
> net/ncsi/ncsi-rsp.c | 71 ++++++++++++++--
> 7 files changed, 363 insertions(+), 9 deletions(-)
>
> diff --git a/include/uapi/linux/ncsi.h b/include/uapi/linux/ncsi.h
> index 4c292ec..4992bfc 100644
> --- a/include/uapi/linux/ncsi.h
> +++ b/include/uapi/linux/ncsi.h
> @@ -30,6 +30,7 @@ enum ncsi_nl_commands {
> NCSI_CMD_PKG_INFO,
> NCSI_CMD_SET_INTERFACE,
> NCSI_CMD_CLEAR_INTERFACE,
> + NCSI_CMD_SEND_CMD,
>
> __NCSI_CMD_AFTER_LAST,
> NCSI_CMD_MAX = __NCSI_CMD_AFTER_LAST - 1
> @@ -43,6 +44,7 @@ enum ncsi_nl_commands {
> * @NCSI_ATTR_PACKAGE_LIST: nested array of NCSI_PKG_ATTR attributes
> * @NCSI_ATTR_PACKAGE_ID: package ID
> * @NCSI_ATTR_CHANNEL_ID: channel ID
> + * @NCSI_ATTR_DATA: command payload
> * @NCSI_ATTR_MAX: highest attribute number
> */
> enum ncsi_nl_attrs {
> @@ -51,6 +53,7 @@ enum ncsi_nl_attrs {
> NCSI_ATTR_PACKAGE_LIST,
> NCSI_ATTR_PACKAGE_ID,
> NCSI_ATTR_CHANNEL_ID,
> + NCSI_ATTR_DATA,
>
> __NCSI_ATTR_AFTER_LAST,
> NCSI_ATTR_MAX = __NCSI_ATTR_AFTER_LAST - 1
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 8055e39..1a3ef9e 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -171,6 +171,8 @@ struct ncsi_package;
> #define NCSI_RESERVED_CHANNEL 0x1f
> #define NCSI_CHANNEL_INDEX(c) ((c) & ((1 << NCSI_PACKAGE_SHIFT) - 1))
> #define NCSI_TO_CHANNEL(p, c) (((p) << NCSI_PACKAGE_SHIFT) | (c))
> +#define NCSI_MAX_PACKAGE 8
> +#define NCSI_MAX_CHANNEL 32
>
> struct ncsi_channel {
> unsigned char id;
> @@ -215,12 +217,17 @@ struct ncsi_request {
> unsigned char id; /* Request ID - 0 to 255 */
> bool used; /* Request that has been assigned */
> unsigned int flags; /* NCSI request property */
> -#define NCSI_REQ_FLAG_EVENT_DRIVEN 1
> +#define NCSI_REQ_FLAG_EVENT_DRIVEN 1
> +#define NCSI_REQ_FLAG_NETLINK_DRIVEN 2
> struct ncsi_dev_priv *ndp; /* Associated NCSI device */
> struct sk_buff *cmd; /* Associated NCSI command packet */
> struct sk_buff *rsp; /* Associated NCSI response packet */
> struct timer_list timer; /* Timer on waiting for response */
> bool enabled; /* Time has been enabled or not */
> +
> + u32 snd_seq; /* netlink sending sequence number */
> + u32 snd_portid; /* netlink portid of sender */
> + struct nlmsghdr nlhdr; /* netlink message header */
> };
>
> enum {
> @@ -305,6 +312,9 @@ struct ncsi_cmd_arg {
> unsigned short words[8];
> unsigned int dwords[4];
> };
> +
> + unsigned char *data; /* Netlink data */
> + struct genl_info *info; /* Netlink information */
> };
>
> extern struct list_head ncsi_dev_list;
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index 7567ca63..43b544c 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -17,6 +17,7 @@
> #include <net/ncsi.h>
> #include <net/net_namespace.h>
> #include <net/sock.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> @@ -211,6 +212,39 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> return 0;
> }
>
> +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> + struct ncsi_cmd_arg *nca)
> +{
> + struct ncsi_cmd_pkt *cmd;
> + unsigned char *dest, *source;
> + unsigned short len;
> +
> + /* struct ncsi_cmd_pkt = minimum length
> + * - frame checksum
> + * - Ethernet header
> + * = 64 - 4 - 14 = 46
> + * minimum payload = 46 - ncsi header - ncsi checksum
> + * = 46 - 16 - 4 = 26
> + */
> + len = nca->payload;
> +
> + /* minimum payload length is 26 bytes to meet minimum packet
> + * length 64
> + */
> + if (len < 26)
> + cmd = skb_put_zero(skb, sizeof(*cmd));
> + else
> + cmd = skb_put_zero(skb, len + sizeof(struct ncsi_pkt_hdr) + 4);
> +
> + dest = (unsigned char *)cmd + sizeof(struct ncsi_pkt_hdr);
> + source = (unsigned char *)nca->data + sizeof(struct ncsi_pkt_hdr);
> + memcpy(dest, source, len);
> +
> + ncsi_cmd_build_header(&cmd->cmd.common, nca);
> +
> + return 0;
> +}
> +
> static struct ncsi_cmd_handler {
> unsigned char type;
> int payload;
> @@ -244,7 +278,7 @@ static struct ncsi_cmd_handler {
> { NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
> { NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
> { NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_OEM, 0, NULL },
> + { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
> { NCSI_PKT_CMD_PLDM, 0, NULL },
> { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> };
> @@ -317,11 +351,20 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> }
>
> /* Get packet payload length and allocate the request */
> - nca->payload = nch->payload;
> + if (nch->payload >= 0)
> + nca->payload = nch->payload;
> +
> nr = ncsi_alloc_command(nca);
> if (!nr)
> return -ENOMEM;
>
> + /* track netlink information */
> + if (nca->req_flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + nr->snd_seq = nca->info->snd_seq;
> + nr->snd_portid = nca->info->snd_portid;
> + nr->nlhdr = *nca->info->nlhdr;
> + }
> +
> /* Prepare the packet */
> nca->id = nr->id;
> ret = nch->handler(nr->cmd, nca);
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 0912847..29f33a1 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -19,6 +19,7 @@
> #include <net/addrconf.h>
> #include <net/ipv6.h>
> #include <net/if_inet6.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> @@ -406,8 +407,13 @@ static void ncsi_request_timeout(struct timer_list *t)
> {
> struct ncsi_request *nr = from_timer(nr, t, timer);
> struct ncsi_dev_priv *ndp = nr->ndp;
> + struct ncsi_package *np;
> + struct ncsi_channel *nc;
> + struct ncsi_cmd_pkt *cmd;
> unsigned long flags;
>
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> /* If the request already had associated response,
> * let the response handler to release it.
> */
> @@ -415,10 +421,26 @@ static void ncsi_request_timeout(struct timer_list *t)
> nr->enabled = false;
> if (nr->rsp || !nr->cmd) {
> spin_unlock_irqrestore(&ndp->lock, flags);
> +
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: %s - early return\n", __func__);
> +
> return;
> }
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> + if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + if (nr->cmd) {
> + /* Find the package */
> + cmd = (struct ncsi_cmd_pkt *)
> + skb_network_header(nr->cmd);
> + ncsi_find_package_and_channel(ndp,
> + cmd->cmd.common.channel,
> + &np, &nc);
> + ncsi_send_netlink_timeout(nr, np, nc);
> + }
> + }
> +
> /* Release the request */
> ncsi_free_request(nr);
> }
> diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
> index 45f33d6..ce57675 100644
> --- a/net/ncsi/ncsi-netlink.c
> +++ b/net/ncsi/ncsi-netlink.c
> @@ -20,6 +20,7 @@
> #include <uapi/linux/ncsi.h>
>
> #include "internal.h"
> +#include "ncsi-pkt.h"
> #include "ncsi-netlink.h"
>
> static struct genl_family ncsi_genl_family;
> @@ -29,6 +30,7 @@ static const struct nla_policy ncsi_genl_policy[NCSI_ATTR_MAX + 1] = {
> [NCSI_ATTR_PACKAGE_LIST] = { .type = NLA_NESTED },
> [NCSI_ATTR_PACKAGE_ID] = { .type = NLA_U32 },
> [NCSI_ATTR_CHANNEL_ID] = { .type = NLA_U32 },
> + [NCSI_ATTR_DATA] = { .type = NLA_BINARY, .len = 2048 },
> };
>
> static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex)
> @@ -366,6 +368,203 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info)
> return 0;
> }
>
> +static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
> +{
> + struct ncsi_dev_priv *ndp;
> +
> + struct ncsi_cmd_arg nca;
> + struct ncsi_pkt_hdr *hdr;
> +
> + u32 package_id, channel_id;
> + unsigned char *data;
> + void *head;
> + int len, ret;
> +
> + if (!info || !info->attrs) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!info->attrs[NCSI_ATTR_IFINDEX]) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!info->attrs[NCSI_ATTR_PACKAGE_ID]) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!info->attrs[NCSI_ATTR_CHANNEL_ID]) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
> + nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
> + if (!ndp) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
> + channel_id = nla_get_u32(info->attrs[NCSI_ATTR_CHANNEL_ID]);
> +
> + if (package_id >= NCSI_MAX_PACKAGE || channel_id >= NCSI_MAX_CHANNEL) {
> + ret = -ERANGE;
> + goto out_netlink;
> + }
> +
> + len = nla_len(info->attrs[NCSI_ATTR_DATA]);
> + if (len < sizeof(struct ncsi_pkt_hdr)) {
> + netdev_info(ndp->ndev.dev, "NCSI: no OEM command to send %u\n",
> + package_id);
> + ret = -EINVAL;
> + goto out_netlink;
> + } else {
> + head = nla_data(info->attrs[NCSI_ATTR_DATA]);
> + data = (unsigned char *)head;
> + }
> +
> + hdr = (struct ncsi_pkt_hdr *)data;
> +
> + nca.ndp = ndp;
> + nca.package = (unsigned char)package_id;
> + nca.channel = (unsigned char)channel_id;
> + nca.type = hdr->type;
> + nca.req_flags = NCSI_REQ_FLAG_NETLINK_DRIVEN;
> + nca.info = info;
> + nca.payload = ntohs(hdr->length);
> + nca.data = data;
> +
> + ret = ncsi_xmit_cmd(&nca);
> +out_netlink:
> + if (ret != 0) {
> + netdev_err(ndp->ndev.dev,
> + "Error %d sending OEM command\n", ret);
> + ncsi_send_netlink_err(ndp->ndev.dev,
> + info->snd_seq,
> + info->snd_portid,
> + info->nlhdr,
> + ret);
> + }
> +out:
> + return ret;
> +}
> +
> +int ncsi_send_netlink_rsp(struct ncsi_request *nr,
> + struct ncsi_package *np,
> + struct ncsi_channel *nc)
> +{
> + struct sk_buff *skb;
> + struct net *net;
> + void *hdr;
> + int rc;
> +
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> + net = dev_net(nr->rsp->dev);
> +
> + skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!skb)
> + return -ENOMEM;
> +
> + hdr = genlmsg_put(skb, nr->snd_portid, nr->snd_seq,
> + &ncsi_genl_family, 0, NCSI_CMD_SEND_CMD);
> + if (!hdr) {
> + kfree_skb(skb);
> + return -EMSGSIZE;
> + }
> +
> + nla_put_u32(skb, NCSI_ATTR_IFINDEX, nr->rsp->dev->ifindex);
> + if (np)
> + nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID, np->id);
> + if (nc)
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, nc->id);
> + else
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, NCSI_RESERVED_CHANNEL);
> +
> + rc = nla_put(skb, NCSI_ATTR_DATA, nr->rsp->len, (void *)nr->rsp->data);
> + if (rc)
> + goto err;
> +
> + genlmsg_end(skb, hdr);
> + return genlmsg_unicast(net, skb, nr->snd_portid);
> +
> +err:
> + kfree_skb(skb);
> + return rc;
> +}
> +
> +int ncsi_send_netlink_timeout(struct ncsi_request *nr,
> + struct ncsi_package *np,
> + struct ncsi_channel *nc)
> +{
> + struct sk_buff *skb;
> + struct net *net;
> + void *hdr;
> +
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> + skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!skb)
> + return -ENOMEM;
> +
> + hdr = genlmsg_put(skb, nr->snd_portid, nr->snd_seq,
> + &ncsi_genl_family, 0, NCSI_CMD_SEND_CMD);
> + if (!hdr) {
> + kfree_skb(skb);
> + return -EMSGSIZE;
> + }
> +
> + net = dev_net(nr->cmd->dev);
> +
> + nla_put_u32(skb, NCSI_ATTR_IFINDEX, nr->cmd->dev->ifindex);
> +
> + if (np)
> + nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID, np->id);
> + else
> + nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID,
> + NCSI_PACKAGE_INDEX((((struct ncsi_pkt_hdr *)
> + nr->cmd->data)->channel)));
> +
> + if (nc)
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, nc->id);
> + else
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, NCSI_RESERVED_CHANNEL);
> +
> + genlmsg_end(skb, hdr);
> + return genlmsg_unicast(net, skb, nr->snd_portid);
> +}
> +
> +int ncsi_send_netlink_err(struct net_device *dev,
> + u32 snd_seq,
> + u32 snd_portid,
> + struct nlmsghdr *nlhdr,
> + int err)
> +{
> + struct sk_buff *skb;
> + struct nlmsghdr *nlh;
> + struct nlmsgerr *nle;
> + struct net *net;
> +
> + skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!skb)
> + return -ENOMEM;
> +
> + net = dev_net(dev);
> +
> + nlh = nlmsg_put(skb, snd_portid, snd_seq,
> + NLMSG_ERROR, sizeof(*nle), 0);
> + nle = (struct nlmsgerr *)nlmsg_data(nlh);
> + nle->error = err;
> + memcpy(&nle->msg, nlhdr, sizeof(*nlh));
> +
> + nlmsg_end(skb, nlh);
> +
> + return nlmsg_unicast(net->genl_sock, skb, snd_portid);
> +}
> +
> static const struct genl_ops ncsi_ops[] = {
> {
> .cmd = NCSI_CMD_PKG_INFO,
> @@ -386,6 +585,12 @@ static const struct genl_ops ncsi_ops[] = {
> .doit = ncsi_clear_interface_nl,
> .flags = GENL_ADMIN_PERM,
> },
> + {
> + .cmd = NCSI_CMD_SEND_CMD,
> + .policy = ncsi_genl_policy,
> + .doit = ncsi_send_cmd_nl,
> + .flags = GENL_ADMIN_PERM,
> + },
> };
>
> static struct genl_family ncsi_genl_family __ro_after_init = {
> diff --git a/net/ncsi/ncsi-netlink.h b/net/ncsi/ncsi-netlink.h
> index 91a5c25..c4a4688 100644
> --- a/net/ncsi/ncsi-netlink.h
> +++ b/net/ncsi/ncsi-netlink.h
> @@ -14,6 +14,18 @@
>
> #include "internal.h"
>
> +int ncsi_send_netlink_rsp(struct ncsi_request *nr,
> + struct ncsi_package *np,
> + struct ncsi_channel *nc);
> +int ncsi_send_netlink_timeout(struct ncsi_request *nr,
> + struct ncsi_package *np,
> + struct ncsi_channel *nc);
> +int ncsi_send_netlink_err(struct net_device *dev,
> + u32 snd_seq,
> + u32 snd_portid,
> + struct nlmsghdr *nlhdr,
> + int err);
> +
> int ncsi_init_netlink(struct net_device *dev);
> int ncsi_unregister_netlink(struct net_device *dev);
>
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 930c1d3..010970f 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -16,9 +16,11 @@
> #include <net/ncsi.h>
> #include <net/net_namespace.h>
> #include <net/sock.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> +#include "ncsi-netlink.h"
>
> static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
> unsigned short payload)
> @@ -32,15 +34,22 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
> * before calling this function.
> */
> h = (struct ncsi_rsp_pkt_hdr *)skb_network_header(nr->rsp);
> - if (h->common.revision != NCSI_PKT_REVISION)
> +
> + if (h->common.revision != NCSI_PKT_REVISION) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: unsupported header revision\n");
> return -EINVAL;
> - if (ntohs(h->common.length) != payload)
> + }
> + if (ntohs(h->common.length) != payload) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: payload length mismatched\n");
> return -EINVAL;
> + }
>
> /* Check on code and reason */
> if (ntohs(h->code) != NCSI_PKT_RSP_C_COMPLETED ||
> - ntohs(h->reason) != NCSI_PKT_RSP_R_NO_ERROR)
> - return -EINVAL;
> + ntohs(h->reason) != NCSI_PKT_RSP_R_NO_ERROR) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: non zero response/reason code\n");
> + return -EPERM;
> + }
>
> /* Validate checksum, which might be zeroes if the
> * sender doesn't support checksum according to NCSI
> @@ -52,8 +61,11 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
>
> checksum = ncsi_calculate_checksum((unsigned char *)h,
> sizeof(*h) + payload - 4);
> - if (*pchecksum != htonl(checksum))
> +
> + if (*pchecksum != htonl(checksum)) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: checksum mismatched\n");
> return -EINVAL;
> + }
>
> return 0;
> }
> @@ -900,6 +912,31 @@ static int ncsi_rsp_handler_gpuuid(struct ncsi_request *nr)
> return 0;
> }
>
> +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> +{
> + return 0;
> +}
> +
> +static int ncsi_rsp_handler_netlink(struct ncsi_request *nr)
> +{
> + struct ncsi_rsp_pkt *rsp;
> + struct ncsi_dev_priv *ndp = nr->ndp;
> + struct ncsi_package *np;
> + struct ncsi_channel *nc;
> + int ret;
> +
> + /* Find the package */
> + rsp = (struct ncsi_rsp_pkt *)skb_network_header(nr->rsp);
> + ncsi_find_package_and_channel(ndp, rsp->rsp.common.channel,
> + &np, &nc);
> + if (!np)
> + return -ENODEV;
> +
> + ret = ncsi_send_netlink_rsp(nr, np, nc);
> +
> + return ret;
> +}
> +
> static struct ncsi_rsp_handler {
> unsigned char type;
> int payload;
> @@ -932,7 +969,7 @@ static struct ncsi_rsp_handler {
> { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> - { NCSI_PKT_RSP_OEM, 0, NULL },
> + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> { NCSI_PKT_RSP_PLDM, 0, NULL },
> { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> };
> @@ -1002,6 +1039,17 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
> netdev_warn(ndp->ndev.dev,
> "NCSI: 'bad' packet ignored for type 0x%x\n",
> hdr->type);
> +
> + if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + if (ret == -EPERM)
> + goto out_netlink;
> + else
> + ncsi_send_netlink_err(ndp->ndev.dev,
> + nr->snd_seq,
> + nr->snd_portid,
> + &nr->nlhdr,
> + ret);
> + }
> goto out;
> }
>
> @@ -1011,6 +1059,17 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
> netdev_err(ndp->ndev.dev,
> "NCSI: Handler for packet type 0x%x returned %d\n",
> hdr->type, ret);
> +
> +out_netlink:
> + if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + ret = ncsi_rsp_handler_netlink(nr);
> + if (ret) {
> + netdev_err(ndp->ndev.dev,
> + "NCSI: Netlink handler for packet type 0x%x returned %d\n",
> + hdr->type, ret);
> + }
> + }
> +
> out:
> ncsi_free_request(nr);
> return ret;
^ permalink raw reply
* [PATCH v2] net/ncsi: Add NCSI OEM command support
From: Christian Svensson @ 2018-10-02 6:29 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1004987ef0ab138e772ed5740a0d839bffe39910.camel@mendozajonas.com>
I'm just going to say that I'm very excited about these patches. Just what
I needed, thanks for contributing them :-).
On Tue, Oct 2, 2018, 03:20 Samuel Mendoza-Jonas <sam@mendozajonas.com>
wrote:
> On Fri, 2018-09-28 at 18:06 -0700, Vijay Khemka wrote:
> > This patch adds OEM commands and response handling. It also defines OEM
> > command and response structure as per NCSI specification along with its
> > handlers.
> >
> > ncsi_cmd_handler_oem: This is a generic command request handler for OEM
> > commands
> > ncsi_rsp_handler_oem: This is a generic response handler for OEM commands
> >
> > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
>
> Hi Vijay - looks good to me, and should be a good common base for your
> and Justin's changes.
>
> Reviewed-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
>
> > ---
> > net/ncsi/internal.h | 4 ++++
> > net/ncsi/ncsi-cmd.c | 31 ++++++++++++++++++++++++++++---
> > net/ncsi/ncsi-pkt.h | 16 ++++++++++++++++
> > net/ncsi/ncsi-rsp.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > 4 files changed, 91 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> > index 8055e3965cef..c16cb7223064 100644
> > --- a/net/ncsi/internal.h
> > +++ b/net/ncsi/internal.h
> > @@ -68,6 +68,10 @@ enum {
> > NCSI_MODE_MAX
> > };
> >
> > +/* OEM Vendor Manufacture ID */
> > +#define NCSI_OEM_MFR_MLX_ID 0x8119
> > +#define NCSI_OEM_MFR_BCM_ID 0x113d
> > +
> > struct ncsi_channel_version {
> > u32 version; /* Supported BCD encoded NCSI version */
> > u32 alpha2; /* Supported BCD encoded NCSI version */
> > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > index 7567ca63aae2..2f98533eba46 100644
> > --- a/net/ncsi/ncsi-cmd.c
> > +++ b/net/ncsi/ncsi-cmd.c
> > @@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff
> *skb,
> > return 0;
> > }
> >
> > +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> > + struct ncsi_cmd_arg *nca)
> > +{
> > + struct ncsi_cmd_oem_pkt *cmd;
> > + unsigned int len;
> > +
> > + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> > + if (nca->payload < 26)
> > + len += 26;
> > + else
> > + len += nca->payload;
> > +
> > + cmd = skb_put_zero(skb, len);
> > + cmd->mfr_id = nca->dwords[0];
> > + memcpy(cmd->data, &nca->dwords[1], nca->payload - 4);
> > + ncsi_cmd_build_header(&cmd->cmd.common, nca);
> > +
> > + return 0;
> > +}
> > +
> > static struct ncsi_cmd_handler {
> > unsigned char type;
> > int payload;
> > @@ -244,7 +264,7 @@ static struct ncsi_cmd_handler {
> > { NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
> > { NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
> > { NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
> > - { NCSI_PKT_CMD_OEM, 0, NULL },
> > + { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
> > { NCSI_PKT_CMD_PLDM, 0, NULL },
> > { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> > };
> > @@ -316,8 +336,13 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> > return -ENOENT;
> > }
> >
> > - /* Get packet payload length and allocate the request */
> > - nca->payload = nch->payload;
> > + /* Get packet payload length and allocate the request
> > + * It is expected that if length set as negative in
> > + * handler structure means caller is initializing it
> > + * and setting length in nca before calling xmit function
> > + */
> > + if (nch->payload >= 0)
> > + nca->payload = nch->payload;
> > nr = ncsi_alloc_command(nca);
> > if (!nr)
> > return -ENOMEM;
> > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > index 91b4b66438df..1f338386810d 100644
> > --- a/net/ncsi/ncsi-pkt.h
> > +++ b/net/ncsi/ncsi-pkt.h
> > @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
> > unsigned char pad[22];
> > };
> >
> > +/* OEM Request Command as per NCSI Specification */
> > +struct ncsi_cmd_oem_pkt {
> > + struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> > + __be32 mfr_id; /* Manufacture ID */
> > + unsigned char data[64]; /* OEM Payload Data */
> > + __be32 checksum; /* Checksum */
> > +};
> > +
> > +/* OEM Response Packet as per NCSI Specification */
> > +struct ncsi_rsp_oem_pkt {
> > + struct ncsi_rsp_pkt_hdr rsp; /* Command header */
> > + __be32 mfr_id; /* Manufacture ID */
> > + unsigned char data[64]; /* Payload data */
> > + __be32 checksum; /* Checksum */
> > +};
> > +
> > /* Get Link Status */
> > struct ncsi_rsp_gls_pkt {
> > struct ncsi_rsp_pkt_hdr rsp; /* Response header */
> > diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> > index 930c1d3796f0..22664ebdc93a 100644
> > --- a/net/ncsi/ncsi-rsp.c
> > +++ b/net/ncsi/ncsi-rsp.c
> > @@ -596,6 +596,48 @@ static int ncsi_rsp_handler_snfc(struct
> ncsi_request *nr)
> > return 0;
> > }
> >
> > +static struct ncsi_rsp_oem_handler {
> > + unsigned int mfr_id;
> > + int (*handler)(struct ncsi_request *nr);
> > +} ncsi_rsp_oem_handlers[] = {
> > + { NCSI_OEM_MFR_MLX_ID, NULL },
> > + { NCSI_OEM_MFR_BCM_ID, NULL }
> > +};
> > +
> > +
> > +/* Response handler for OEM command */
> > +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> > +{
> > + struct ncsi_rsp_oem_pkt *rsp;
> > + struct ncsi_rsp_oem_handler *nrh = NULL;
> > + unsigned int mfr_id, i;
> > +
> > + /* Get the response header */
> > + rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> > + mfr_id = ntohl(rsp->mfr_id);
> > +
> > + /* Check for manufacturer id and Find the handler */
> > + for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
> > + if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
> > + if (ncsi_rsp_oem_handlers[i].handler)
> > + nrh = &ncsi_rsp_oem_handlers[i];
> > + else
> > + nrh = NULL;
> > +
> > + break;
> > + }
> > + }
> > +
> > + if (!nrh) {
> > + netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM
> packet with MFR-ID (0x%x)\n",
> > + mfr_id);
> > + return -ENOENT;
> > + }
> > +
> > + /* Process the packet */
> > + return nrh->handler(nr);
> > +}
> > +
> > static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
> > {
> > struct ncsi_rsp_gvi_pkt *rsp;
> > @@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
> > { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> > { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> > { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> > - { NCSI_PKT_RSP_OEM, 0, NULL },
> > + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> > { NCSI_PKT_RSP_PLDM, 0, NULL },
> > { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> > };
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20181002/487ee8c6/attachment-0001.html>
^ permalink raw reply
* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
From: Linus Walleij @ 2018-10-02 7:35 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181002041437.2493192-1-taoren@fb.com>
Hi Tao, thanks for your patch!
On Tue, Oct 2, 2018 at 6:14 AM Tao Ren <taoren@fb.com> wrote:
> - if (fttmr010->count_down)
> + if (fttmr010->count_down) {
> writel(~0, fttmr010->base + TIMER1_LOAD);
This struct member "count_down" is a bit badly named now don't
you think?
The patch is fine semantically, but please rename this member
"is_aspeed" or something like that and update the code everywhere,
then insert a comments like
/* The Aspeed variant counts downward */
/* The Aspeed variant does not have a match interrupt */
in the code snippets so we see what is going on.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v2] net/ncsi: Add NCSI OEM command support
From: Justin.Lee1 @ 2018-10-02 16:53 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1004987ef0ab138e772ed5740a0d839bffe39910.camel@mendozajonas.com>
Hi Vijay,
Looks good. Please see my comment below.
Thanks,
Justin
> On Fri, 2018-09-28 at 18:06 -0700, Vijay Khemka wrote:
> > This patch adds OEM commands and response handling. It also defines OEM
> > command and response structure as per NCSI specification along with its
> > handlers.
> >
> > ncsi_cmd_handler_oem: This is a generic command request handler for OEM
> > commands
> > ncsi_rsp_handler_oem: This is a generic response handler for OEM commands
> >
> > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
>
> Hi Vijay - looks good to me, and should be a good common base for your
> and Justin's changes.
>
> Reviewed-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
>
> > ---
> > net/ncsi/internal.h | 4 ++++
> > net/ncsi/ncsi-cmd.c | 31 ++++++++++++++++++++++++++++---
> > net/ncsi/ncsi-pkt.h | 16 ++++++++++++++++
> > net/ncsi/ncsi-rsp.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > 4 files changed, 91 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> > index 8055e3965cef..c16cb7223064 100644
> > --- a/net/ncsi/internal.h
> > +++ b/net/ncsi/internal.h
> > @@ -68,6 +68,10 @@ enum {
> > NCSI_MODE_MAX
> > };
> >
> > +/* OEM Vendor Manufacture ID */
> > +#define NCSI_OEM_MFR_MLX_ID 0x8119
> > +#define NCSI_OEM_MFR_BCM_ID 0x113d
> > +
> > struct ncsi_channel_version {
> > u32 version; /* Supported BCD encoded NCSI version */
> > u32 alpha2; /* Supported BCD encoded NCSI version */
> > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > index 7567ca63aae2..2f98533eba46 100644
> > --- a/net/ncsi/ncsi-cmd.c
> > +++ b/net/ncsi/ncsi-cmd.c
> > @@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> > return 0;
> > }
> >
> > +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> > + struct ncsi_cmd_arg *nca)
> > +{
> > + struct ncsi_cmd_oem_pkt *cmd;
OEM command doesn't not have the fixed data size. Should we use pointer instead?
struct ncsi_cmd_oem_pkt {
struct ncsi_cmd_pkt_hdr cmd; /* Command header */
__be32 mfr_id; /* Manufacture ID */
unsigned char *data; /* OEM Payload Data */
};
> > + unsigned int len;
> > +
> > + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> > + if (nca->payload < 26)
> > + len += 26;
Why does it add 26? I knew the other place in ncsi_alloc_command() is also add 26.
If it is less than 26, then it should be a fixed size of structure ncsi_cmd_pkt (46), right?
> > + else
> > + len += nca->payload;
> > +
> > + cmd = skb_put_zero(skb, len);
> > + cmd->mfr_id = nca->dwords[0];
> > + memcpy(cmd->data, &nca->dwords[1], nca->payload - 4);
Netlink request is using the new nca->data pointer to pass the data as the request payload
is not the same size and some command payload is bigger than 16 bytes.
Will you consider to use the same data pointer? So, we don't need to have a checking here.
If the command is used less than 16 bytes, we can simply assigned &nca->bytes[0] to it.
> > + ncsi_cmd_build_header(&cmd->cmd.common, nca);
> > +
> > + return 0;
> > +}
> > +
> > static struct ncsi_cmd_handler {
> > unsigned char type;
> > int payload;
> > @@ -244,7 +264,7 @@ static struct ncsi_cmd_handler {
> > { NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
> > { NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
> > { NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
> > - { NCSI_PKT_CMD_OEM, 0, NULL },
> > + { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
> > { NCSI_PKT_CMD_PLDM, 0, NULL },
> > { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> > };
> > @@ -316,8 +336,13 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> > return -ENOENT;
> > }
> >
> > - /* Get packet payload length and allocate the request */
> > - nca->payload = nch->payload;
> > + /* Get packet payload length and allocate the request
> > + * It is expected that if length set as negative in
> > + * handler structure means caller is initializing it
> > + * and setting length in nca before calling xmit function
> > + */
> > + if (nch->payload >= 0)
> > + nca->payload = nch->payload;
> > nr = ncsi_alloc_command(nca);
> > if (!nr)
> > return -ENOMEM;
> > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > index 91b4b66438df..1f338386810d 100644
> > --- a/net/ncsi/ncsi-pkt.h
> > +++ b/net/ncsi/ncsi-pkt.h
> > @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
> > unsigned char pad[22];
> > };
> >
> > +/* OEM Request Command as per NCSI Specification */
> > +struct ncsi_cmd_oem_pkt {
> > + struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> > + __be32 mfr_id; /* Manufacture ID */
> > + unsigned char data[64]; /* OEM Payload Data */
> > + __be32 checksum; /* Checksum */
> > +};
> > +
> > +/* OEM Response Packet as per NCSI Specification */
> > +struct ncsi_rsp_oem_pkt {
> > + struct ncsi_rsp_pkt_hdr rsp; /* Command header */
> > + __be32 mfr_id; /* Manufacture ID */
> > + unsigned char data[64]; /* Payload data */
> > + __be32 checksum; /* Checksum */
> > +};
> > +
OEM command doesn't not have the fixed response data size too.
Should we use pointer instead?
> > /* Get Link Status */
> > struct ncsi_rsp_gls_pkt {
> > struct ncsi_rsp_pkt_hdr rsp; /* Response header */
> > diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> > index 930c1d3796f0..22664ebdc93a 100644
> > --- a/net/ncsi/ncsi-rsp.c
> > +++ b/net/ncsi/ncsi-rsp.c
> > @@ -596,6 +596,48 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
> > return 0;
> > }
> >
> > +static struct ncsi_rsp_oem_handler {
> > + unsigned int mfr_id;
> > + int (*handler)(struct ncsi_request *nr);
> > +} ncsi_rsp_oem_handlers[] = {
> > + { NCSI_OEM_MFR_MLX_ID, NULL },
> > + { NCSI_OEM_MFR_BCM_ID, NULL }
> > +};
> > +
> > +
> > +/* Response handler for OEM command */
> > +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> > +{
> > + struct ncsi_rsp_oem_pkt *rsp;
> > + struct ncsi_rsp_oem_handler *nrh = NULL;
> > + unsigned int mfr_id, i;
> > +
> > + /* Get the response header */
> > + rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> > + mfr_id = ntohl(rsp->mfr_id);
> > +
> > + /* Check for manufacturer id and Find the handler */
> > + for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
> > + if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
> > + if (ncsi_rsp_oem_handlers[i].handler)
> > + nrh = &ncsi_rsp_oem_handlers[i];
> > + else
> > + nrh = NULL;
> > +
> > + break;
> > + }
> > + }
> > +
> > + if (!nrh) {
> > + netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM packet with MFR-ID (0x%x)\n",
> > + mfr_id);
> > + return -ENOENT;
> > + }
> > +
> > + /* Process the packet */
> > + return nrh->handler(nr);
> > +}
> > +
> > static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
> > {
> > struct ncsi_rsp_gvi_pkt *rsp;
> > @@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
> > { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> > { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> > { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> > - { NCSI_PKT_RSP_OEM, 0, NULL },
> > + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> > { NCSI_PKT_RSP_PLDM, 0, NULL },
> > { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> > };
^ permalink raw reply
* [PATCH net v2] net/ncsi: Extend NC-SI Netlink interface to allow user space to send NC-SI command
From: Justin.Lee1 @ 2018-10-02 17:36 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <18e571bb7c5879a12754b1df20f51fd87ea48712.camel@mendozajonas.com>
Hi Sam,
Sure, I will generate v3 after Vijay's patch is approved.
Thanks,
Justin
> On Fri, 2018-09-28 at 18:15 +0000, Justin.Lee1 at Dell.com wrote:
> > The new command (NCSI_CMD_SEND_CMD) is added to allow user space application
> > to send NC-SI command to the network card.
> > Also, add a new attribute (NCSI_ATTR_DATA) for transferring request and response.
> >
> > The work flow is as below.
> >
> > Request:
> > User space application -> Netlink interface (msg)
> > -> new Netlink handler - ncsi_send_cmd_nl()
> > -> ncsi_xmit_cmd()
> > Response:
> > Response received - ncsi_rcv_rsp() -> internal response handler - ncsi_rsp_handler_xxx()
> > -> ncsi_rsp_handler_netlink()
> > -> ncsi_send_netlink_rsp ()
> > -> Netlink interface (msg)
> > -> user space application
> > Command timeout - ncsi_request_timeout() -> ncsi_send_netlink_timeout ()
> > -> Netlink interface (msg with zero data length)
> > -> user space application
> > Error:
> > Error detected -> ncsi_send_netlink_err () -> Netlink interface (err msg)
> > -> user space application
> >
> >
> > Signed-off-by: Justin Lee <justin.lee1@dell.com>
>
> Hi Justin,
>
> This is looking pretty good, combined with Vijay's base patch the two
> approaches should fit together nicely (
> http://patchwork.ozlabs.org/patch/976510/).
>
> A good merge order would probably be the above patch first, then this
> patch and Vijay's further OEM patches based on top of that to reduce
> conflicts.
>
> Cheers,
> Sam
^ permalink raw reply
* [PATCH v2] net/ncsi: Add NCSI OEM command support
From: Vijay Khemka @ 2018-10-02 18:56 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <4876838166d140bab331280efcfd771d@AUSX13MPS306.AMER.DELL.COM>
Hi Justin,
Thanks for response. Please see my comments below.
-Vijay
> > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > index 7567ca63aae2..2f98533eba46 100644
> > --- a/net/ncsi/ncsi-cmd.c
> > +++ b/net/ncsi/ncsi-cmd.c
> > @@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> > return 0;
> > }
> >
> > +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> > + struct ncsi_cmd_arg *nca)
> > +{
> > + struct ncsi_cmd_oem_pkt *cmd;
>OEM command doesn't not have the fixed data size. Should we use pointer instead?
>struct ncsi_cmd_oem_pkt {
> struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> __be32 mfr_id; /* Manufacture ID */
> unsigned char *data; /* OEM Payload Data */
>};
Yes, I agree that OEM command doesn't have fixed data but to map to skbuff structure,
I have defined above structure as per NCSI spec, We can certainly have a MAX_DATA_LEN define.
> > + unsigned int len;
> > +
> > + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> > + if (nca->payload < 26)
> > + len += 26;
>Why does it add 26? I knew the other place in ncsi_alloc_command() is also add 26.
>If it is less than 26, then it should be a fixed size of structure ncsi_cmd_pkt (46), right?
It adds 26 because It has already assigned len to hdr+4 which is 16+4 = 20 bytes. By
adding 26 it makes it to 46. I am just being consistent with other portion of code.
> > + else
> > + len += nca->payload;
> > +
> > + cmd = skb_put_zero(skb, len);
> > + cmd->mfr_id = nca->dwords[0];
> > + memcpy(cmd->data, &nca->dwords[1], nca->payload - 4);
>Netlink request is using the new nca->data pointer to pass the data as the request payload
>is not the same size and some command payload is bigger than 16 bytes.
>Will you consider to use the same data pointer? So, we don't need to have a checking here.
>If the command is used less than 16 bytes, we can simply assigned &nca->bytes[0] to it.
To keep original structure, we can change 16 bytes to MAX_DATA_LEN. Or I don't see any issue in
Copying data from data pointer from nca but user needs to be aware if it is less than 16 bytes then
use bytes or use data pointer. To keep it simple, we should simply define MAX_LEN.
> > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > index 91b4b66438df..1f338386810d 100644
> > --- a/net/ncsi/ncsi-pkt.h
> > +++ b/net/ncsi/ncsi-pkt.h
> > @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
> > unsigned char pad[22];
> > };
> >
> > +/* OEM Request Command as per NCSI Specification */
> > +struct ncsi_cmd_oem_pkt {
> > + struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> > + __be32 mfr_id; /* Manufacture ID */
> > + unsigned char data[64]; /* OEM Payload Data */
> > + __be32 checksum; /* Checksum */
> > +};
> > +
> > +/* OEM Response Packet as per NCSI Specification */
> > +struct ncsi_rsp_oem_pkt {
> > + struct ncsi_rsp_pkt_hdr rsp; /* Command header */
> > + __be32 mfr_id; /* Manufacture ID */
> > + unsigned char data[64]; /* Payload data */
> > + __be32 checksum; /* Checksum */
> > +};
> > +
>OEM command doesn't not have the fixed response data size too.
>Should we use pointer instead?
Here also we can define MAX_DATA_LEN because data pointer won't map to skb directly.
^ permalink raw reply
* [PATCH v2] net/ncsi: Add NCSI OEM command support
From: Justin.Lee1 @ 2018-10-02 20:53 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <F932552B-5751-48E9-BC8B-6F9D7841C7FA@fb.com>
Hi Vijay,
Please see the comments below.
Thanks,
Justin
> Hi Justin,
> Thanks for response. Please see my comments below.
>
> -Vijay
>
> > > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > > index 7567ca63aae2..2f98533eba46 100644
> > > --- a/net/ncsi/ncsi-cmd.c
> > > +++ b/net/ncsi/ncsi-cmd.c
> > > @@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> > > return 0;
> > > }
> > >
> > > +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> > > + struct ncsi_cmd_arg *nca)
> > > +{
> > > + struct ncsi_cmd_oem_pkt *cmd;
>
> >OEM command doesn't not have the fixed data size. Should we use pointer instead?
> >struct ncsi_cmd_oem_pkt {
> > struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> > __be32 mfr_id; /* Manufacture ID */
> > unsigned char *data; /* OEM Payload Data */
> >};
> Yes, I agree that OEM command doesn't have fixed data but to map to skbuff structure,
> I have defined above structure as per NCSI spec, We can certainly have a MAX_DATA_LEN define.
The spec only defines manufacture ID field and the start offset of vendor data.
If we just want to point to the vendor data, we don't need to specify the max length and
we can use flexible array as the last member (correct the typo above).
struct ncsi_cmd_oem_pkt {
struct ncsi_cmd_pkt_hdr cmd; /* Command header */
__be32 mfr_id; /* Manufacture ID */
unsigned char data[]; /* OEM Payload Data */
};
> > > + unsigned int len;
> > > +
> > > + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> > > + if (nca->payload < 26)
> > > + len += 26;
>
> >Why does it add 26? I knew the other place in ncsi_alloc_command() is also add 26.
> >If it is less than 26, then it should be a fixed size of structure ncsi_cmd_pkt (46), right?
> It adds 26 because It has already assigned len to hdr+4 which is 16+4 = 20 bytes. By
> adding 26 it makes it to 46. I am just being consistent with other portion of code.
>
> > > + else
> > > + len += nca->payload;
> > > +
> > > + cmd = skb_put_zero(skb, len);
> > > + cmd->mfr_id = nca->dwords[0];
> > > + memcpy(cmd->data, &nca->dwords[1], nca->payload - 4);
>
> >Netlink request is using the new nca->data pointer to pass the data as the request payload
> >is not the same size and some command payload is bigger than 16 bytes.
> >Will you consider to use the same data pointer? So, we don't need to have a checking here.
> >If the command is used less than 16 bytes, we can simply assigned &nca->bytes[0] to it.
> To keep original structure, we can change 16 bytes to MAX_DATA_LEN. Or I don't see any issue in
> Copying data from data pointer from nca but user needs to be aware if it is less than 16 bytes then
> use bytes or use data pointer. To keep it simple, we should simply define MAX_LEN.
I use the pointer to avoid copying the data again. OEM handler can always process the flexible payload
through the data pointer. It can depend on the caller to use stack/allocated buffer/nca buffer and
it will be straight forward if all OEM stuff is using the data pointer to process the flexible
payload.
>
> > > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > > index 91b4b66438df..1f338386810d 100644
> > > --- a/net/ncsi/ncsi-pkt.h
> > > +++ b/net/ncsi/ncsi-pkt.h
> > > @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
> > > unsigned char pad[22];
> > > };
> > >
> > > +/* OEM Request Command as per NCSI Specification */
> > > +struct ncsi_cmd_oem_pkt {
> > > + struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> > > + __be32 mfr_id; /* Manufacture ID */
> > > + unsigned char data[64]; /* OEM Payload Data */
> > > + __be32 checksum; /* Checksum */
> > > +};
> > > +
> > > +/* OEM Response Packet as per NCSI Specification */
> > > +struct ncsi_rsp_oem_pkt {
> > > + struct ncsi_rsp_pkt_hdr rsp; /* Command header */
> > > + __be32 mfr_id; /* Manufacture ID */
> > > + unsigned char data[64]; /* Payload data */
> > > + __be32 checksum; /* Checksum */
> > > +};
> > > +
>
> >OEM command doesn't not have the fixed response data size too.
> >Should we use pointer instead?
>
> Here also we can define MAX_DATA_LEN because data pointer won't map to skb directly.
Suggest to change as below due to the flexible response.
struct ncsi_rsp_oem_pkt {
struct ncsi_rsp_pkt_hdr rsp; /* Command header */
__be32 mfr_id; /* Manufacture ID */
unsigned char data[]; /* Payload data */
};
^ permalink raw reply
* [PATCH v2] net/ncsi: Add NCSI OEM command support
From: Vijay Khemka @ 2018-10-02 21:34 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <3dc1e3c74a864c3da7aaf19c3ca9edfa@AUSX13MPS306.AMER.DELL.COM>
Hi Justin,
Please see comments below
-Vijay
> > > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > > index 7567ca63aae2..2f98533eba46 100644
> > > --- a/net/ncsi/ncsi-cmd.c
> > > +++ b/net/ncsi/ncsi-cmd.c
> > > @@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> > > return 0;
> > > }
> > >
> > > +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> > > + struct ncsi_cmd_arg *nca)
> > > +{
> > > + struct ncsi_cmd_oem_pkt *cmd;
>
> >OEM command doesn't not have the fixed data size. Should we use pointer instead?
> >struct ncsi_cmd_oem_pkt {
> > struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> > __be32 mfr_id; /* Manufacture ID */
> > unsigned char *data; /* OEM Payload Data */
> >};
> Yes, I agree that OEM command doesn't have fixed data but to map to skbuff structure,
> I have defined above structure as per NCSI spec, We can certainly have a MAX_DATA_LEN define.
The spec only defines manufacture ID field and the start offset of vendor data.
If we just want to point to the vendor data, we don't need to specify the max length and
we can use flexible array as the last member (correct the typo above).
struct ncsi_cmd_oem_pkt {
struct ncsi_cmd_pkt_hdr cmd; /* Command header */
__be32 mfr_id; /* Manufacture ID */
unsigned char data[]; /* OEM Payload Data */
};
I agree and will add this change.
> > > + unsigned int len;
> > > +
> > > + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> > > + if (nca->payload < 26)
> > > + len += 26;
>
> >Why does it add 26? I knew the other place in ncsi_alloc_command() is also add 26.
> >If it is less than 26, then it should be a fixed size of structure ncsi_cmd_pkt (46), right?
> It adds 26 because It has already assigned len to hdr+4 which is 16+4 = 20 bytes. By
> adding 26 it makes it to 46. I am just being consistent with other portion of code.
>
> > > + else
> > > + len += nca->payload;
> > > +
> > > + cmd = skb_put_zero(skb, len);
> > > + cmd->mfr_id = nca->dwords[0];
> > > + memcpy(cmd->data, &nca->dwords[1], nca->payload - 4);
>
> >Netlink request is using the new nca->data pointer to pass the data as the request payload
> >is not the same size and some command payload is bigger than 16 bytes.
> >Will you consider to use the same data pointer? So, we don't need to have a checking here.
> >If the command is used less than 16 bytes, we can simply assigned &nca->bytes[0] to it.
> To keep original structure, we can change 16 bytes to MAX_DATA_LEN. Or I don't see any issue in
> Copying data from data pointer from nca but user needs to be aware if it is less than 16 bytes then
> use bytes or use data pointer. To keep it simple, we should simply define MAX_LEN.
I use the pointer to avoid copying the data again. OEM handler can always process the flexible payload
through the data pointer. It can depend on the caller to use stack/allocated buffer/nca buffer and
it will be straight forward if all OEM stuff is using the data pointer to process the flexible
payload.
Do we use data pointer for payload less than 16 bytes as well in OEM. We copy data only once while adding to skbuff.
I have no issue, I will make this change but wait for other comments.
>
> > > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > > index 91b4b66438df..1f338386810d 100644
> > > --- a/net/ncsi/ncsi-pkt.h
> > > +++ b/net/ncsi/ncsi-pkt.h
> > > @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
> > > unsigned char pad[22];
> > > };
> > >
> > > +/* OEM Request Command as per NCSI Specification */
> > > +struct ncsi_cmd_oem_pkt {
> > > + struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> > > + __be32 mfr_id; /* Manufacture ID */
> > > + unsigned char data[64]; /* OEM Payload Data */
> > > + __be32 checksum; /* Checksum */
> > > +};
> > > +
> > > +/* OEM Response Packet as per NCSI Specification */
> > > +struct ncsi_rsp_oem_pkt {
> > > + struct ncsi_rsp_pkt_hdr rsp; /* Command header */
> > > + __be32 mfr_id; /* Manufacture ID */
> > > + unsigned char data[64]; /* Payload data */
> > > + __be32 checksum; /* Checksum */
> > > +};
> > > +
>
> >OEM command doesn't not have the fixed response data size too.
> >Should we use pointer instead?
>
> Here also we can define MAX_DATA_LEN because data pointer won't map to skb directly.
Suggest to change as below due to the flexible response.
struct ncsi_rsp_oem_pkt {
struct ncsi_rsp_pkt_hdr rsp; /* Command header */
__be32 mfr_id; /* Manufacture ID */
unsigned char data[]; /* Payload data */
};
Sure done.
^ permalink raw reply
* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
From: Tao Ren @ 2018-10-02 22:08 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACRpkdY4VaH2Yoob2gWwAk11P1NTDdza262zKGs=gju0NH9skQ@mail.gmail.com>
On 10/02/2018 12:35 AM, Linus Walleij wrote:
> This struct member "count_down" is a bit badly named now don't you think?
> The patch is fine semantically, but please rename this member "is_aspeed" or something like that and update the code everywhere,
Thank you Linus for the quick review.
I was actually planning a cleanup patch which handles is_aspeed/count_down stuff. Basically we have 2 options:
??? 1) adding "is_aspeed" flag. "count_down" is kept because potentially faraday-fttmr could also be configured as count_down timer (although I don't see any reason to do so).
??? 2) renaming "count_down" to "is_aspeed". By doing this, we set restriction that faraday-fttmr will always be upward.
What's your thought? Do you want me to include all the changes in this diff?
> then insert a comments like
> /* The Aspeed variant counts downward */
> /* The Aspeed variant does not have a match interrupt */
> in the code snippets so we see what is going on.
Make sense. Let me add more comments.
Thanks,
Tao Ren
^ permalink raw reply
* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
From: Linus Walleij @ 2018-10-03 6:56 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <3a294c92-bcc6-b42e-b3af-72bc5f607fd9@fb.com>
On Wed, Oct 3, 2018 at 12:08 AM Tao Ren <taoren@fb.com> wrote:
> 1) adding "is_aspeed" flag. "count_down" is kept because potentially faraday-fttmr could also be configured as count_down timer (although I don't see any reason to do so).
> 2) renaming "count_down" to "is_aspeed". By doing this, we set restriction that faraday-fttmr will always be upward.
> What's your thought? Do you want me to include all the changes in this diff?
My thought is go for (2) and do all changes in one patch :)
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
From: Tao Ren @ 2018-10-03 7:46 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACRpkdZXum+bQg1FO+EHhKWpxtPYXfeRhoAKkFXeMQh1Jng3Ww@mail.gmail.com>
On 10/2/18, 11:57 PM, "Linus Walleij" <linus.walleij@linaro.org> wrote:
> My thought is go for (2) and do all changes in one patch :)
No problem, Linus.
One more question: looks like my first patch 4451d3f59f2a (fix set_next_event handler) is not merged back to "timers/core". Should I generate this patch on top of the first patch or on top of the current "timers/core"? Which one would be easier for you (or Daniel/Thomas)? Sorry I'm pretty new to the community..
Thanks,
Tao Ren
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox