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.
