Re: [PATCH 1/1] mcb: Remove requesting memory region to avoid memory overlapping

[Rodríguez Barbarin, José Javier]

On Thu, 2023-03-23 at 15:25 +0100, Johannes Thumshirn wrote:
> On 23.03.23 13:49, Rodríguez Barbarin, José Javier wrote:
> > mcb-pci requests a fixed-size memory region to parse the chameleon
> > table, however, if the chameleon table is smaller that the
> > allocated
> > region, it could overlaps with the IP Cores' memory regions.
> > 
> > There is no need to request the memory region for only parsing the
> > chameleon table as it is read-only. It is sufficient to only remaps
> > it when parsing.
> > 
> > Co-developed-by: Jorge Sanjuan Garcia
> > <jorge.sanjuangarcia@xxxxxxxxxx>
> > Signed-off-by: Jorge Sanjuan Garcia
> > <jorge.sanjuangarcia@xxxxxxxxxx>
> > Signed-off-by: Javier Rodriguez <josejavier.rodriguez@xxxxxxxxxx>
> > ---
> >  drivers/mcb/mcb-pci.c | 10 ----------
> >  1 file changed, 10 deletions(-)
> > 
> > diff --git a/drivers/mcb/mcb-pci.c b/drivers/mcb/mcb-pci.c
> > index dc88232d9af8..e98f626020a7 100644
> > --- a/drivers/mcb/mcb-pci.c
> > +++ b/drivers/mcb/mcb-pci.c
> > @@ -29,7 +29,6 @@ static int mcb_pci_get_irq(struct mcb_device
> > *mdev)
> >  
> >  static int mcb_pci_probe(struct pci_dev *pdev, const struct
> > pci_device_id *id)
> >  {
> > -       struct resource *res;
> >         struct priv *priv;
> >         int ret;
> >         unsigned long flags;
> > @@ -52,15 +51,6 @@ static int mcb_pci_probe(struct pci_dev *pdev,
> > const struct pci_device_id *id)
> >                 goto out_disable;
> >         }
> >  
> > -       res = devm_request_mem_region(&pdev->dev, priv->mapbase,
> > -                                     CHAM_HEADER_SIZE,
> > -                                     KBUILD_MODNAME);
> > -       if (!res) {
> > -               dev_err(&pdev->dev, "Failed to request PCI
> > memory\n");
> > -               ret = -EBUSY;
> > -               goto out_disable;
> > -       }
> > -
> >         priv->base = devm_ioremap(&pdev->dev, priv->mapbase,
> > CHAM_HEADER_SIZE);
> >         if (!priv->base) {
> >                 dev_err(&pdev->dev, "Cannot ioremap\n");
> 
> TBH I'm not sure I like this approach.
> 
> One thing that could be done (and probably the easiest and most
> correct)
> is resizing
> the resource used for the header. Aka checking the header size and
> dropping/reallocating
> it once we have the correct size. It's not a fastpath anyways so we
> don't really care about
> reallocations here.
> 
> Another way would be flagging these ressources as IORESOURCE_MUXED
> but
> that would need
> quite some plumbing, especially for the 16z125 as it uses 8250_port
> for
> requesting memory.
> 
> If these are specific FPGA versions/revisions, maybe they could be
> quirked as well so we only
> allocate as much space as we need.
> 
> I'm not too deep into resource manage any more, but
> Documentation/PCI/pci.rst explicitly
> states, that a driver must request the region to verify no other
> device
> is alreay using the
> resource.
> 
> So until you (or someone else) are proofing me wrong on these I'd say
> NACK to this.
> 
> Byte,
>     Johannes

Hi Johannes,

One thing we can do is modify the chameleon_parse_cells prototype to
return the actual "chameleon table" size instead of the number of cells
which can have an undetermined size. At the moment, the return value is
only used for error checking but the number of cells is never used.

After that, we can check the actual "chameleon table" size and
drop/reallocate if needed.

Regards,

Javier