Received-SPF: pass (google.com: domain of linux-kernel+bounces-203464-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240605212840.3227774-1-olvaffe@gmail.com> <ZmDiYze9MEFxn50C@smile.fi.intel.com>
 <CAPaKu7SJ5TVXsqnsqnucpE=wVKwGCdXyCTNBhr+rcJrmLw5GgQ@mail.gmail.com> <ZmD2c0rNz10KsPA0@smile.fi.intel.com>
In-Reply-To: <ZmD2c0rNz10KsPA0@smile.fi.intel.com>
From: Chia-I Wu <olvaffe@gmail.com>
Date: Wed, 5 Jun 2024 17:42:05 -0700
Message-ID: <CAPaKu7Rt_wn4nycc=2hr6N6X77vDqyvFbkgrBeEzfmY3nEaong@mail.gmail.com>
Subject: Re: [PATCH v4] resource: add a simple test for walk_iomem_res_desc()
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Dan Williams <dan.j.williams@intel.com>, 
	AKASHI Takahiro <takahiro.akashi@linaro.org>, Dave Jiang <dave.jiang@intel.com>, 
	Alison Schofield <alison.schofield@intel.com>, Baoquan He <bhe@redhat.com>, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 5, 2024 at 4:36=E2=80=AFPM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Jun 05, 2024 at 03:52:26PM -0700, Chia-I Wu wrote:
> > On Wed, Jun 5, 2024 at 3:10=E2=80=AFPM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Jun 05, 2024 at 02:28:26PM -0700, Chia-I Wu wrote:
> > > > This mainly tests that find_next_iomem_res() does not miss resource=
s.
>
> ...
>
> > > > +     /* build the resource tree */
> > > > +     res[0] =3D DEFINE_RES_NAMED(root.start + 0x0000, 0x1000, "SYS=
RAM 1",
> > > > +                     IORESOURCE_SYSTEM_RAM);
> > > > +     res[1] =3D DEFINE_RES_NAMED(root.start + 0x1000, 0x1000, "OTH=
ER", 0);
> > > > +
> > > > +     res[2] =3D DEFINE_RES_NAMED(root.start + 0x3000, 0x1000, "NES=
TED", 0);
> > > > +     res[3] =3D DEFINE_RES_NAMED(root.start + 0x3800, 0x0400, "SYS=
RAM 2",
> > > > +                     IORESOURCE_SYSTEM_RAM);
> > >
> > > ...here is overlap with the previous resource.
> > >
> > > And here is the gap to the next one, in case we make that overlapping=
 gone.
> > >
> > > > +     res[4] =3D DEFINE_RES_NAMED(root.start + 0x4000, 0x1000, "SYS=
RAM 3",
> > > > +                     IORESOURCE_SYSTEM_RAM);
> > >
> > > It wasn't the case in previous data. Please, elaborate what's going o=
n here?
> > The test data is chosen to be
> >
> >   first interval: a matching resource (res[0])
> >   second interval: a non-matching resource (res[1])
> >   third interval: a hole
> >   fourth interval: a matching resource (res[3]) nested in a
> > non-matching resource (res[2])
> >   fifth interval: a matching resource (res[4])
> >
> > The idea hasn't changed between revisions.
> >
> > res[3] went from a half of res[2] to a quarter of res[2] in v4.  I
> > guess it causes confusion if it is not viewed as a nested resource.
>
> Okay, so far it's correct data from testing p.o.v.
>
> Maybe you can add a comment on top explaining this layout?
Done in v5.  I also added negative tests for all holes in the test
data.  Hope it's better now.

>
> ...
>
> > > And rather sending one version per 12h, take your time and think more=
 about
> > > test data. What are we testing? Are the testing data correct? Shouldn=
't we also
> > > have negative test cases?
> > The current choice of test data covers the most common patterns.  Do
> > you have other patterns you want to cover?  I am new to the resource
> > code and that's why I am largely reactive to review feedback.
>
> Nope, seems okay to have what is there for the starter. Later on we might=
 add
> more if required. Just got confused.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>