On Fri, Feb 28, 2025 at 05:35:53PM -0800, Shannon Nelson wrote:
> + /* reject unsupported and/or out of scope commands */
> + op_entry = (struct pds_fwctl_query_data_operation *)ep_info->operations->entries;
> + for (i = 0; i < ep_info->operations->num_entries; i++) {
> + if (PDS_FWCTL_RPC_OPCODE_CMP(rpc->in.op, op_entry[i].id)) {
> + if (scope < op_entry[i].scope)
> + return -EPERM;
> + return 0;
Oh I see, you are doing the scope like CXL with a "command intent's
report". That is a good idea.
> + if (rpc->in.len > 0) {
> + in_payload = kzalloc(rpc->in.len, GFP_KERNEL);
> + if (!in_payload) {
> + dev_err(dev, "Failed to allocate in_payload\n");
> + out = ERR_PTR(-ENOMEM);
> + goto done;
> + }
> +
> + if (copy_from_user(in_payload, u64_to_user_ptr(rpc->in.payload),
> + rpc->in.len)) {
> + dev_err(dev, "Failed to copy in_payload from user\n");
> + out = ERR_PTR(-EFAULT);
> + goto done;
> + }
This wasn't really the intention to have a payload pointer inside the
existing payload pointer, is it the same issue as CXL where you wanted
a header?
I see your FW API also can't take a scatterlist, so it makes sense it
needs to be linearized like this.
I don't think it makes a difference to have the indirection or not, it
would be a bunch of stuff to keep the header seperate from the payload
and then still also end up with memcpy in the driver, so leave it.
> +#define PDS_FWCTL_RPC_OPCODE_CMD_SHIFT 0
> +#define PDS_FWCTL_RPC_OPCODE_CMD_MASK GENMASK(15, PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)
> +#define PDS_FWCTL_RPC_OPCODE_VER_SHIFT 16
> +#define PDS_FWCTL_RPC_OPCODE_VER_MASK GENMASK(23, PDS_FWCTL_RPC_OPCODE_VER_SHIFT)
> +
> +#define PDS_FWCTL_RPC_OPCODE_GET_CMD(op) \
> + (((op) & PDS_FWCTL_RPC_OPCODE_CMD_MASK) >> PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)
Isn't that FIELD_GET?
> +struct fwctl_rpc_pds {
> + struct {
> + __u32 op;
> + __u32 ep;
> + __u32 rsvd;
> + __u32 len;
> + __u64 payload;
> + } in;
> + struct {
> + __u32 retval;
> + __u32 rsvd[2];
> + __u32 len;
> + __u64 payload;
> + } out;
> +};
Use __aligned_u64 in the uapi headers
Jason
