Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Richter, Robert" <Robert.Richter@cavium.com>
To:     Julien Thierry <julien.thierry@arm.com>
CC:     Marc Zyngier <marc.zyngier@arm.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jason Cooper <jason@lakedaemon.net>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Stuart Yoder <stuyoder@gmail.com>,
        Laurentiu Tudor <laurentiu.tudor@nxp.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Will Deacon <will.deacon@arm.com>,
        Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Subject: Re: [PATCH 07/10] irqchip/gic-v3-its: Split probing from its node
 initialization
Thread-Topic: [PATCH 07/10] irqchip/gic-v3-its: Split probing from its node
 initialization
Thread-Index: AQHUduW4a8Xj9OqBxEGOxPHNQ+pN26VFvbAAgAFYegA=
Date:   Fri, 9 Nov 2018 07:58:30 +0000
Message-ID: <20181109075820.GD4064@rric.localdomain>
References: <20181107220254.6116-1-rrichter@cavium.com>
 <20181107220254.6116-8-rrichter@cavium.com>
 <3ab57b7c-43e4-f6b8-fa4e-433fd087fc01@arm.com>
In-Reply-To: <3ab57b7c-43e4-f6b8-fa4e-433fd087fc01@arm.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [2.245.13.105]
x-ms-publictraffictype: Email
x-microsoft-exchange-diagnostics: 1;SN6PR07MB5295;6:eWQtncu2ew14mPpY1zDo7aBYJ4y6ILxdaUhZbxhhZpEgv+TqVeBAla7A8DYlTQfaNnEnEbDIqh6D6TvK2zfA2/Yd10eu7E6u3pEOTZBzoxtwpFbkiSoiF5FzFaX6nnQ2CQGfaM5Y0zrPfaGOKyqacMLr642gFUXCNJJq8fSr+hGgl72rihPU3qZMsDOg124gJwU1LpK+LOBVlH7YT5uf0QrFkDUh7FJM6nTYcp1cje2ENYZ01ulz+QJpupcusYVPY7BLwQgravg4ZWKqoXeQNfuJEdTpvFtyE2aD+E3XcDXcfJ5WKiVf1zSpvO3TLRXxLHFxukhurnQOirq+xVzvh6sr5rKSDUN2RJPEjCttDpvH2w5jkvgfuGS8GoCNTMxo3G2pj94Uw3KOMh0sFZSfr7gpZuUJA/9HxeSAjjslFZVFImarzM49LCiXYYsCDQm5U6iziGhAvNCOsgcM8qFLEw==;5:WrS7ReG+C8EYH2koA+jMmJTW9mo6/V3hyi1/3nMkO5G1h/9lw+OxUvTXRY2mmD9oqShs+cBlg7BhrwC3i+Ul/LbHL4jrp4TEuF1MAEjEee/TtEKSfIZd5lHxHbmQ2QEvqgtFtOr9jfQm7sajvHVxZ0z/DWRfwKRgtMwIXbzjT2I=;7:TjeLhH2D7osFUVSxLIYAUundXyuBchYqIOpc02E1N/drHkk55ZB8MA6PwL0ZI8Yc9aK8J8FJATEOGBaJ11heecUzddXfARdd0lXJlOYCXLW7qNfxc7unG69qecffWRXwhZaa7N7rKsXNjUVro7dDwg==
x-ms-office365-filtering-correlation-id: b3d9c69b-bbee-414e-4f27-08d64619233c
x-microsoft-antispam: BCL:0;PCL:0;RULEID:(7020095)(4652040)(8989299)(5600074)(711020)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7153060)(7193020);SRVR:SN6PR07MB5295;
x-ms-traffictypediagnostic: SN6PR07MB5295:
x-microsoft-antispam-prvs: <SN6PR07MB5295EC1696198E5BB08FFADEF1C60@SN6PR07MB5295.namprd07.prod.outlook.com>
x-exchange-antispam-report-test: UriScan:;
x-ms-exchange-senderadcheck: 1
x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(93006095)(93001095)(10201501046)(3002001)(3231382)(944501410)(52105095)(148016)(149066)(150057)(6041310)(20161123564045)(20161123560045)(20161123562045)(20161123558120)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(201708071742011)(7699051)(76991095);SRVR:SN6PR07MB5295;BCL:0;PCL:0;RULEID:;SRVR:SN6PR07MB5295;
x-forefront-prvs: 08512C5403
x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(136003)(39860400002)(346002)(376002)(366004)(396003)(199004)(189003)(72206003)(5660300001)(14444005)(486006)(68736007)(53546011)(186003)(305945005)(316002)(54906003)(26005)(97736004)(14454004)(52116002)(11346002)(6116002)(6506007)(386003)(478600001)(3846002)(446003)(66066001)(2900100001)(1076002)(476003)(256004)(7416002)(6246003)(53936002)(99286004)(25786009)(8676002)(71200400001)(33656002)(2906002)(229853002)(9686003)(6486002)(39060400002)(4326008)(81156014)(6916009)(7736002)(102836004)(33896004)(81166006)(86362001)(76176011)(106356001)(8936002)(71190400001)(6436002)(6512007)(105586002);DIR:OUT;SFP:1101;SCL:1;SRVR:SN6PR07MB5295;H:SN6PR07MB5326.namprd07.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1;
received-spf: None (protection.outlook.com: cavium.com does not designate
 permitted sender hosts)
x-microsoft-antispam-message-info: Qsbf3KiH4ixi5+IAO5+kmiY7pbBaPPopyfnolJitlDoGOcKi73uLEmum8s9p99Hq9iy2h7WTNuEGgiFbdHOdqbJwgIsuVM87+pt7fHYeiH3ftk0YIDmL2idWswKNLQBhe9h99BJZ6TyE9fayb/h+Z7s6R0woGzMhxLZRFe9QzUybIhUt1p2GvEFYrRJrjeSSVYceS7vLK2k3fEVdS11t7Og1zG09gW8B1j043FHFuw1BiYEkxOCHueo4Y+1GgdExw+4+AK/BnbOenENliokStiYkZBFm0bF3it0sPvehV9VJlB5fduT+G1V9C8TYNIw/c2oHuwzzYnv+woI8d11yntJDxZKgKycahh5CuyZ18nk=
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-ID: <6614ABC5315A38458C6F167C31DE358D@namprd07.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: cavium.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b3d9c69b-bbee-414e-4f27-08d64619233c
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Nov 2018 07:58:30.4369
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 711e4ccf-2e9b-4bcf-a551-4094005b6194
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR07MB5295
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 08.11.18 11:25:24, Julien Thierry wrote:
> On 07/11/18 22:03, Robert Richter wrote:

> >-static int its_init_domain(struct fwnode_handle *handle, struct its_nod=
e *its)
> >+static int its_init_domain(struct its_node *its)
> >  {
> >      struct irq_domain *inner_domain;
> >      struct msi_domain_info *info;
> >@@ -3384,7 +3385,8 @@ static int its_init_domain(struct fwnode_handle *h=
andle, struct its_node *its)
> >      if (!info)
> >              return -ENOMEM;
> >
> >-     inner_domain =3D irq_domain_create_tree(handle, &its_domain_ops, i=
ts);
> >+     inner_domain =3D irq_domain_create_tree(its->fwnode_handle,
> >+                                     &its_domain_ops, its);
>=20
> Separate change?
>=20
> >      if (!inner_domain) {
> >              kfree(info);
> >              return -ENOMEM;
> >@@ -3441,8 +3443,7 @@ static int its_init_vpe_domain(void)
> >      return 0;
> >  }
> >
> >-static int __init its_compute_its_list_map(struct resource *res,
> >-                                        void __iomem *its_base)
> >+static int __init its_compute_its_list_map(struct its_node *its)
> >  {
> >      int its_number;
> >      u32 ctlr;
> >@@ -3456,15 +3457,15 @@ static int __init its_compute_its_list_map(struc=
t resource *res,
> >      its_number =3D find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_M=
AX);
> >      if (its_number >=3D GICv4_ITS_LIST_MAX) {
> >              pr_err("ITS@%pa: No ITSList entry available!\n",
> >-                    &res->start);
> >+                    &its->phys_base);
> >              return -EINVAL;
> >      }
> >
> >-     ctlr =3D readl_relaxed(its_base + GITS_CTLR);
> >+     ctlr =3D readl_relaxed(its->base + GITS_CTLR);
> >      ctlr &=3D ~GITS_CTLR_ITS_NUMBER;
> >      ctlr |=3D its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
> >-     writel_relaxed(ctlr, its_base + GITS_CTLR);
> >-     ctlr =3D readl_relaxed(its_base + GITS_CTLR);
> >+     writel_relaxed(ctlr, its->base + GITS_CTLR);
> >+     ctlr =3D readl_relaxed(its->base + GITS_CTLR);
>=20
> This (removal of its_base parameter) also feel like a separate change.

In a separate change the motivation of the change would not be
obvious. While the change of the variable itself is trivial from the
perspective of review and testing, I decided to keep it in the context
of the overall change of this patch.

>=20
> Also, I would define a local variable its_base to avoid dereferencing
> its every time in order to get the base address.

Hmm, there is not much difference in reading the code then, while the
use of a local variable just adds more code without benefit. The
compiler does not care as the value is probably stored in a register
anyway. There are also other struct members, should all of them being
mirrored in a local variable?

>=20
> >      if ((ctlr & GITS_CTLR_ITS_NUMBER) !=3D (its_number << GITS_CTLR_IT=
S_NUMBER_SHIFT)) {
> >              its_number =3D ctlr & GITS_CTLR_ITS_NUMBER;
> >              its_number >>=3D GITS_CTLR_ITS_NUMBER_SHIFT;
> >@@ -3472,83 +3473,110 @@ static int __init its_compute_its_list_map(stru=
ct resource *res,
> >
> >      if (test_and_set_bit(its_number, &its_list_map)) {
> >              pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
> >-                    &res->start, its_number);
> >+                    &its->phys_base, its_number);
> >              return -EINVAL;
> >      }
> >
> >      return its_number;
> >  }
> >
> >+static void its_free(struct its_node *its)
> >+{
> >+     raw_spin_lock(&its_lock);
> >+     list_del(&its->entry);
> >+     raw_spin_unlock(&its_lock);
> >+
> >+     kfree(its);
> >+}
> >+
> >+static int __init its_init_one(struct its_node *its);
>=20
> You might as well define its_init_one here, no?

This is an intermediate definition that will be removed in a later
patch. Moving the whole code here would make the change less readable.

>=20
> >+
> >  static int __init its_probe_one(struct resource *res,
> >                              struct fwnode_handle *handle, int numa_nod=
e)
> >  {
> >      struct its_node *its;
> >+     int err;
> >+
> >+     its =3D kzalloc(sizeof(*its), GFP_KERNEL);
> >+     if (!its)
> >+             return -ENOMEM;
> >+
> >+     raw_spin_lock_init(&its->lock);
> >+     INIT_LIST_HEAD(&its->entry);
> >+     INIT_LIST_HEAD(&its->its_device_list);
> >+     its->fwnode_handle =3D handle;
> >+     its->phys_base =3D res->start;
> >+     its->phys_size =3D resource_size(res);
> >+     its->numa_node =3D numa_node;
> >+
> >+     raw_spin_lock(&its_lock);
> >+     list_add_tail(&its->entry, &its_nodes);
> >+     raw_spin_unlock(&its_lock);
> >+
> >+     pr_info("ITS %pR\n", res);
> >+
> >+     err =3D its_init_one(its);
> >+     if (err)
> >+             its_free(its);
> >+
> >+     return err;
> >+}
> >+
> >+static int __init its_init_one(struct its_node *its)
> >+{
> >      void __iomem *its_base;
> >      u32 val, ctlr;
> >      u64 baser, tmp, typer;
> >      int err;
> >
> >-     its_base =3D ioremap(res->start, resource_size(res));
> >+     its_base =3D ioremap(its->phys_base, its->phys_size);
> >      if (!its_base) {
> >-             pr_warn("ITS@%pa: Unable to map ITS registers\n", &res->st=
art);
> >-             return -ENOMEM;
> >+             pr_warn("ITS@%pa: Unable to map ITS registers\n", &its->ph=
ys_base);
> >+             err =3D -ENOMEM;
> >+             goto fail;
> >      }
> >
> >      val =3D readl_relaxed(its_base + GITS_PIDR2) & GIC_PIDR2_ARCH_MASK=
;
> >      if (val !=3D 0x30 && val !=3D 0x40) {
> >-             pr_warn("ITS@%pa: No ITS detected, giving up\n", &res->sta=
rt);
> >+             pr_warn("ITS@%pa: No ITS detected, giving up\n", &its->phy=
s_base);
> >              err =3D -ENODEV;
> >              goto out_unmap;
> >      }
> >
> >      err =3D its_force_quiescent(its_base);
> >      if (err) {
> >-             pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &res->s=
tart);
> >+             pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &its->p=
hys_base);
> >              goto out_unmap;
> >      }
> >
> >-     pr_info("ITS %pR\n", res);
> >-
> >-     its =3D kzalloc(sizeof(*its), GFP_KERNEL);
> >-     if (!its) {
> >-             err =3D -ENOMEM;
> >-             goto out_unmap;
> >-     }
> >-
> >-     raw_spin_lock_init(&its->lock);
> >-     INIT_LIST_HEAD(&its->entry);
> >-     INIT_LIST_HEAD(&its->its_device_list);
> >      typer =3D gic_read_typer(its_base + GITS_TYPER);
> >      its->base =3D its_base;
> >-     its->phys_base =3D res->start;
> >      its->ite_size =3D GITS_TYPER_ITT_ENTRY_SIZE(typer);
> >      its->device_ids =3D GITS_TYPER_DEVBITS(typer);
> >      its->is_v4 =3D !!(typer & GITS_TYPER_VLPIS);
> >      if (its->is_v4) {
> >              if (!(typer & GITS_TYPER_VMOVP)) {
> >-                     err =3D its_compute_its_list_map(res, its_base);
> >+                     err =3D its_compute_its_list_map(its);
> >                      if (err < 0)
> >-                             goto out_free_its;
> >+                             goto out_unmap;
> >
> >                      its->list_nr =3D err;
> >
> >                      pr_info("ITS@%pa: Using ITS number %d\n",
> >-                             &res->start, err);
> >+                             &its->phys_base, err);
> >              } else {
> >-                     pr_info("ITS@%pa: Single VMOVP capable\n", &res->s=
tart);
> >+                     pr_info("ITS@%pa: Single VMOVP capable\n",
> >+                             &its->phys_base);
> >              }
> >      }
> >
> >-     its->numa_node =3D numa_node;
> >-
> >      its->cmd_base =3D (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO=
,
> >                                              get_order(ITS_CMD_QUEUE_SZ=
));
> >      if (!its->cmd_base) {
> >              err =3D -ENOMEM;
> >-             goto out_free_its;
> >+             goto out_unmap;
> >      }
> >      its->cmd_write =3D its->cmd_base;
> >-     its->fwnode_handle =3D handle;
> >      its->get_msi_base =3D its_irq_get_msi_base;
> >      its->msi_domain_flags =3D IRQ_DOMAIN_FLAG_MSI_REMAP;
> >
> >@@ -3597,13 +3625,11 @@ static int __init its_probe_one(struct resource =
*res,
> >      if (GITS_TYPER_HCC(typer))
> >              its->flags |=3D ITS_FLAGS_SAVE_SUSPEND_STATE;
> >
> >-     err =3D its_init_domain(handle, its);
> >+     err =3D its_init_domain(its);
>=20
> I'm not sure what is the logic for "this goes in probe, this goes in init=
?".

It is fairly simple, gic-its register access is done in init.
Everything that is detected during devicetree or ACPI device discovery
is done in the probe function that collects all data in struct
its_node.

>=20
> >      if (err)
> >              goto out_free_tables;
> >
> >-     raw_spin_lock(&its_lock);
> >-     list_add_tail(&its->entry, &its_nodes);
> >-     raw_spin_unlock(&its_lock);
> >+     pr_info("ITS@%pa: ITS node added\n", &its->phys_base);
> >
> >      return 0;

Thanks,

-Robert