Re: [PATCH v4 1/5] spdm-socket: add seperate send/recv functions

[Wilfred Mallawa <wilfred.mallawa@wdc.com>]

On Thu, 2025-09-04 at 11:10 +0100, Jonathan Cameron wrote:
> > 
[...]
> > diff --git a/backends/spdm-socket.c b/backends/spdm-socket.c
> > index 2c709c68c8..3d264814df 100644
> > --- a/backends/spdm-socket.c
> > +++ b/backends/spdm-socket.c
> > @@ -184,28 +184,62 @@ int spdm_socket_connect(uint16_t port, Error
> > **errp)
> >      return client_socket;
> >  }
> >  
> > -uint32_t spdm_socket_rsp(const int socket, uint32_t
> > transport_type,
> > -                         void *req, uint32_t req_len,
> > -                         void *rsp, uint32_t rsp_len)
> > +static bool spdm_socket_command_valid(uint32_t command)
> As below - perhaps this sanity check belongs in a precursor patch?
> > +{
> > +    switch (command) {
> > +    case SPDM_SOCKET_COMMAND_NORMAL:
> > +    case SPDM_SOCKET_STORAGE_CMD_IF_SEND:
> > +    case SPDM_SOCKET_STORAGE_CMD_IF_RECV:
> > +    case SOCKET_SPDM_STORAGE_ACK_STATUS:
> > +    case SPDM_SOCKET_COMMAND_OOB_ENCAP_KEY_UPDATE:
> > +    case SPDM_SOCKET_COMMAND_CONTINUE:
> > +    case SPDM_SOCKET_COMMAND_SHUTDOWN:
> > +    case SPDM_SOCKET_COMMAND_UNKOWN:
> > +    case SPDM_SOCKET_COMMAND_TEST:
> > +        return true;
> > +    default:
> > +        return false;
> > +    }
> > +}
> > +
> > +uint32_t spdm_socket_receive(const int socket, uint32_t
> > transport_type,
> > +                             void *rsp, uint32_t rsp_len)
> >  {
> >      uint32_t command;
> >      bool result;
> >  
> > -    result = send_platform_data(socket, transport_type,
> > -                                SPDM_SOCKET_COMMAND_NORMAL,
> > -                                req, req_len);
> > -    if (!result) {
> > +    result = receive_platform_data(socket, transport_type,
> > &command,
> > +                                   (uint8_t *)rsp, &rsp_len);
> > +
> > +    /* we may have received some data, but check if the command is
> > valid */
> > +    if (!result || !spdm_socket_command_valid(command)) {
> 
> Is this change related to the separate send/recv part?  Perhaps it
> is a useful bit of hardening to do as a precursor patch?
Hey Jonathan,

I think it makes sense to integrate this directly into
spdm_socket_receive() in this patch. As in a precursor patch, it would
need to be changed here again.
> 
> >          return 0;
> >      }
> >  
> > -    result = receive_platform_data(socket, transport_type,
> > &command,
> > -                                   (uint8_t *)rsp, &rsp_len);
> > +    return rsp_len;
> > +}
> > +
> > +bool spdm_socket_send(const int socket, uint32_t socket_cmd,
> > +                      uint32_t transport_type, void *req, uint32_t
> > req_len)
> > +{
> > +    return send_platform_data(socket, transport_type,
> > +                              socket_cmd, req, req_len);
> 
> I'd wrap that closer to 80 chars.
Ah yep!
> 
> > +}
> > +
> > +uint32_t spdm_socket_rsp(const int socket, uint32_t
> > transport_type,
> > +                         void *req, uint32_t req_len,
> > +                         void *rsp, uint32_t rsp_len)
> > +{
> > +    bool result;
> > +
> > +    result = spdm_socket_send(socket, SPDM_SOCKET_COMMAND_NORMAL,
> > +                              transport_type, req, req_len);
> >      if (!result) {
> >          return 0;
> >      }
> >  
> > -    assert(command != 0);
> > -
> > +    rsp_len = spdm_socket_receive(socket, transport_type, (uint8_t
> > *)rsp,
> 
> Why casting to a uint8_t * ?  It is a void * and this function takes
> a void *.
Okay yeah, will fixup.
> 
> > +                                  rsp_len);
> >      return rsp_len;
> >  }
> >  
> > diff --git a/include/system/spdm-socket.h b/include/system/spdm-
> > socket.h
> > index 5d8bd9aa4e..2b7d03f82d 100644
> > --- a/include/system/spdm-socket.h
> > +++ b/include/system/spdm-socket.h
> 
> >  /**
> >   * spdm_socket_close: send a shutdown command to the server
> >   * @socket: socket returned from spdm_socket_connect()
> > @@ -60,6 +89,9 @@ uint32_t spdm_socket_rsp(const int socket,
> > uint32_t transport_type,
> >  void spdm_socket_close(const int socket, uint32_t transport_type);
> >  
> >  #define SPDM_SOCKET_COMMAND_NORMAL                0x0001
> > +#define SPDM_SOCKET_STORAGE_CMD_IF_SEND           0x0002
> > +#define SPDM_SOCKET_STORAGE_CMD_IF_RECV           0x0003
> > +#define SOCKET_SPDM_STORAGE_ACK_STATUS            0x0004
> >  #define SPDM_SOCKET_COMMAND_OOB_ENCAP_KEY_UPDATE  0x8001
> >  #define SPDM_SOCKET_COMMAND_CONTINUE              0xFFFD
> >  #define SPDM_SOCKET_COMMAND_SHUTDOWN              0xFFFE
> > @@ -68,7 +100,10 @@ void spdm_socket_close(const int socket,
> > uint32_t transport_type);
> >  
> >  #define SPDM_SOCKET_TRANSPORT_TYPE_MCTP           0x01
> >  #define SPDM_SOCKET_TRANSPORT_TYPE_PCI_DOE        0x02
> > +#define SPDM_SOCKET_TRANSPORT_TYPE_SCSI           0x03
> > +#define SPDM_SOCKET_TRANSPORT_TYPE_NVME           0x04
> 
> Not used in this patch. Move it to where it is first used.
> 
> >  
> >  #define SPDM_SOCKET_MAX_MESSAGE_BUFFER_SIZE       0x1200
> > +#define SPDM_SOCKET_MAX_MSG_STATUS_LEN            0x02
> 
> Not used in this patch. 
Sounds good, I will move them up.

Thanks!
Wilfred