linux-kernel - Re: [PATCH 2/8] platform/x86/intel/sdsi: Combine read and write mailbox flows

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <47df451d2494243d73e9065202e50e022ae2792b.camel@linux.intel.com>
Date: Thu, 01 Feb 2024 10:11:54 -0800
From: "David E. Box" <david.e.box@...ux.intel.com>
To: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>,
  netdev@...r.kernel.org, ilpo.jarvinen@...ux.intel.com
Cc: linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 2/8] platform/x86/intel/sdsi: Combine read and write
 mailbox flows

Hi Sathya,

On Thu, 2024-02-01 at 09:31 -0800, Kuppuswamy Sathyanarayanan wrote:
> 
> On 1/31/24 5:07 PM, David E. Box wrote:
> > The current mailbox commands are either read-only or write-only and the
> > flow is different for each. New commands will need to send and receive
> > data. In preparation for these commands, create a common polling function
> > to handle sending data and receiving in the same transaction.
> > 
> > Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> > ---
> >  drivers/platform/x86/intel/sdsi.c | 79 +++++++++++++++++--------------
> >  1 file changed, 44 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/sdsi.c
> > b/drivers/platform/x86/intel/sdsi.c
> > index a70c071de6e2..05a35f2f85b6 100644
> > --- a/drivers/platform/x86/intel/sdsi.c
> > +++ b/drivers/platform/x86/intel/sdsi.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/overflow.h>
> >  #include <linux/pci.h>
> >  #include <linux/slab.h>
> >  #include <linux/sysfs.h>
> > @@ -156,8 +157,8 @@ static int sdsi_status_to_errno(u32 status)
> >         }
> >  }
> >  
> > -static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info
> > *info,
> > -                             size_t *data_size)
> > +static int sdsi_mbox_poll(struct sdsi_priv *priv, struct sdsi_mbox_info
> > *info,
> > +                         size_t *data_size)
> >  {
> >         struct device *dev = priv->dev;
> >         u32 total, loop, eom, status, message_size;
> > @@ -166,18 +167,10 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv,
> > struct sdsi_mbox_info *inf
> >  
> >         lockdep_assert_held(&priv->mb_lock);
> >  
> > -       /* Format and send the read command */
> > -       control = FIELD_PREP(CTRL_EOM, 1) |
> > -                 FIELD_PREP(CTRL_SOM, 1) |
> > -                 FIELD_PREP(CTRL_RUN_BUSY, 1) |
> > -                 FIELD_PREP(CTRL_PACKET_SIZE, info->size);
> > -       writeq(control, priv->control_addr);
> > -
> >         /* For reads, data sizes that are larger than the mailbox size are
> > read in packets. */
> >         total = 0;
> >         loop = 0;
> >         do {
> > -               void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop);
> >                 u32 packet_size;
> >  
> >                 /* Poll on ready bit */
> > @@ -195,6 +188,11 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv,
> > struct sdsi_mbox_info *inf
> >                 if (ret)
> >                         break;
> >  
> > +               if (!packet_size) {
> > +                       sdsi_complete_transaction(priv);
> > +                       break;
> > +               }
> > +
> 
> It seems to be a generic check. Is this related to converting to a read/write
> function or
> a common fix you added together in this patch.

Yes, it's related. The code that follows this only applies to reads. This check
would be true only for writes which only need to poll once to confirm the write
was successful. I'll add a comment to make it clear.