Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Johannes Thumshirn <jth@kernel.org>
Subject: Re: [PATCH 1/1] mcb: Remove requesting memory region to avoid memory
 overlapping
To:     =?UTF-8?Q?Rodr=c3=adguez_Barbarin=2c_Jos=c3=a9_Javier?= 
        <JoseJavier.Rodriguez@duagon.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc:     "jth@kernel.org" <jth@kernel.org>,
        =?UTF-8?B?U2FuanXDoW4gR2FyY8OtYSwgSm9yZ2U=?= 
        <Jorge.SanjuanGarcia@duagon.com>, Michael Moese <mmoese@suse.de>
References: <20230323124900.898035-1-josejavier.rodriguez@duagon.com>
 <20230323124900.898035-2-josejavier.rodriguez@duagon.com>
Message-ID: <c08407ec-8553-87ca-85d5-1988b8f9172b@kernel.org>
Date:   Thu, 23 Mar 2023 15:25:54 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <20230323124900.898035-2-josejavier.rodriguez@duagon.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@duagon.com>
> Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
> Signed-off-by: Javier Rodriguez <josejavier.rodriguez@duagon.com>
> ---
>  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