linux-kernel - Re: [PATCH 07/25] scsi: hisi_sas: add ioremap for device HW

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5074860.NRYCq0Qjjh@wuerfel>
Date:	Tue, 13 Oct 2015 14:20:59 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	zhangfei <zhangfei.gao@...aro.org>
Cc:	John Garry <john.garry@...wei.com>,
	James.Bottomley@...senpartnership.com,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linuxarm@...wei.com, linux-scsi@...r.kernel.org,
	xuwei5@...ilicon.com, john.garry2@...l.dcu.ie, hare@...e.de
Subject: Re: [PATCH 07/25] scsi: hisi_sas: add ioremap for device HW

On Tuesday 13 October 2015 17:47:02 zhangfei wrote:
> On 10/12/2015 11:21 PM, Arnd Bergmann wrote:
> > On Monday 12 October 2015 23:20:19 John Garry wrote:
> >> +int hisi_sas_ioremap(struct hisi_hba *hisi_hba)
> >> +{
> >> +       struct platform_device *pdev = hisi_hba->pdev;
> >> +       struct device *dev = &pdev->dev;
> >> +       struct resource *res;
> >> +
> >> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +       hisi_hba->regs = devm_ioremap(dev,
> >> +                                     res->start,
> >> +                                     resource_size(res));
> >> +       if (!hisi_hba->regs)
> >> +               return -ENOMEM;
> >> +
> >> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >> +       hisi_hba->ctrl_regs = devm_ioremap(dev,
> >> +                                          res->start,
> >> +                                          resource_size(res));
> >> +       if (!hisi_hba->ctrl_regs)
> >> +               return -ENOMEM;
> >> +
> >> +       return 0;
> >> +}
> >>
> >>   static const struct of_device_id sas_of_match[] = {
> >>
> >
> > Better use devm_ioremap_resource() here, which registers the resource so they
> > are checked for conflicts and listed in /proc/iomem.
> >
> 
> Yes, hisi_hba->regs can use devm_ioremap_resource.
> 
> However ctrl_regs have to use devm_ioremap, since the address are 
> sharing among different nodes, unfortunately, and devm_ioremap_resource 
> will fail.

This sounds like it should be fixed in the DT binding then, to ensure
that the ranges don't overlap.

Mapping the same register region multiple times is generally considered
a bad idea because the drivers that map them often don't have global
locks that serialize the access, so it's better to have code in place
that ensures that they are distinct.

What is the purpose of the ctrl_regs region, and why is it shared
across multiple devices?

Are all users of these registers in the same driver?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/