Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 12, 2022 at 03:57:36PM +0100, Jan Beulich wrote:
> On 25.11.2021 12:02, Oleksandr Andrushchenko wrote:
> > @@ -379,7 +396,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
> > unsigned int size)
> >  
> >          data = merge_result(data, tmp_data, size - data_offset, 
> > data_offset);
> >      }
> > -    spin_unlock(&pdev->vpci->lock);
> >  
> >      return data & (0xffffffff >> (32 - 8 * size));
> >  }
> 
> Here and ...
> 
> > @@ -475,13 +498,12 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, 
> > unsigned int size,
> >              break;
> >          ASSERT(data_offset < size);
> >      }
> > +    spin_unlock(&pdev->vpci_lock);
> >  
> >      if ( data_offset < size )
> >          /* Tailing gap, write the remaining. */
> >          vpci_write_hw(sbdf, reg + data_offset, size - data_offset,
> >                        data >> (data_offset * 8));
> > -
> > -    spin_unlock(&pdev->vpci->lock);
> >  }
> 
> ... even more so here I'm not sure of the correctness of the moving
> you do: While pdev->vpci indeed doesn't get accessed, I wonder
> whether there wasn't an intention to avoid racing calls to
> vpci_{read,write}_hw() this way. In any event I think such movement
> would need justification in the description.

I agree about the need for justification in the commit message, or
even better this being split into a pre-patch, as it's not related to
the lock switching done here.

I do think this is fine however, as racing calls to
vpci_{read,write}_hw() shouldn't be a problem. Those are just wrappers
around pci_conf_{read,write} functions, and the required locking (in
case of using the IO ports) is already taken care in
pci_conf_{read,write}.

Thanks, Roger.