Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] i2c: at91: Send bus clear command if SCL or SDA is down
From: kbouhara @ 2019-09-19 15:06 UTC (permalink / raw)
  To: Codrin Ciubotariu, linux-i2c, linux-arm-kernel, linux-kernel
  Cc: ludovic.desroches, alexandre.belloni, wsa
In-Reply-To: <20190911095854.5141-1-codrin.ciubotariu@microchip.com>

On 9/11/19 11:58 AM, Codrin Ciubotariu wrote:
> After a transfer timeout, some faulty I2C slave devices might hold down
> the SCL or the SDA pins. We can generate a bus clear command, hoping that
> the slave might release the pins.
>
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> ---
>   drivers/i2c/busses/i2c-at91-master.c | 20 ++++++++++++++++++++
>   drivers/i2c/busses/i2c-at91.h        |  6 +++++-
>   2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index a3fcc35ffd3b..5f544a16db96 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -599,6 +599,26 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
>   		at91_twi_write(dev, AT91_TWI_CR,
>   			       AT91_TWI_THRCLR | AT91_TWI_LOCKCLR);
>   	}
> +
> +	/*
> +	 * After timeout, some faulty I2C slave devices might hold SCL/SDA down;
> +	 * we can send a bus clear command, hoping that the pins will be
> +	 * released
> +	 */
> +	if (!(dev->transfer_status & AT91_TWI_SDA) ||
> +	    !(dev->transfer_status & AT91_TWI_SCL)) {
> +		dev_dbg(dev->dev,
> +			"SDA/SCL are down; sending bus clear command\n");
> +		if (dev->use_alt_cmd) {
> +			unsigned int acr;
> +
> +			acr = at91_twi_read(dev, AT91_TWI_ACR);
> +			acr &= ~AT91_TWI_ACR_DATAL_MASK;
> +			at91_twi_write(dev, AT91_TWI_ACR, acr);
> +		}
> +		at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_CLEAR);

This bit is not documented on SoCs before SAMA5D2/D4, this write 
shouldn't be done unconditionally.


-- 
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 6/8] iommu/arm-smmu-v3: Support auxiliary domains
From: Jean-Philippe Brucker @ 2019-09-19 15:06 UTC (permalink / raw)
  To: Will Deacon
  Cc: mark.rutland, devicetree, jacob.jun.pan, Jean-Philippe Brucker,
	joro, linux-kernel, eric.auger, iommu, robh+dt, robin.murphy,
	linux-arm-kernel
In-Reply-To: <20190626175959.ubxvb2qn4taclact@willie-the-truck>

On Wed, Jun 26, 2019 at 06:59:59PM +0100, Will Deacon wrote:
> > @@ -666,8 +668,14 @@ struct arm_smmu_domain {
> >  
> >  	struct iommu_domain		domain;
> >  
> > +	/* Unused in aux domains */
> >  	struct list_head		devices;
> >  	spinlock_t			devices_lock;
> > +
> > +	/* Auxiliary domain stuff */
> > +	struct arm_smmu_domain		*parent;
> > +	ioasid_t			ssid;
> > +	unsigned long			aux_nr_devs;
> 
> Maybe use a union to avoid comments about what is used/unused?

OK

> > +static void arm_smmu_aux_detach_dev(struct iommu_domain *domain, struct device *dev)
> > +{
> > +	struct iommu_domain *parent_domain;
> > +	struct arm_smmu_domain *parent_smmu_domain;
> > +	struct arm_smmu_master *master = dev_to_master(dev);
> > +	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > +
> > +	if (!arm_smmu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX))
> > +		return;
> > +
> > +	parent_domain = iommu_get_domain_for_dev(dev);
> > +	if (!parent_domain)
> > +		return;
> > +	parent_smmu_domain = to_smmu_domain(parent_domain);
> > +
> > +	mutex_lock(&smmu_domain->init_mutex);
> > +	if (!smmu_domain->aux_nr_devs)
> > +		goto out_unlock;
> > +
> > +	if (!--smmu_domain->aux_nr_devs) {
> > +		arm_smmu_write_ctx_desc(parent_smmu_domain, smmu_domain->ssid,
> > +					NULL);
> > +		/*
> > +		 * TLB doesn't need invalidation since accesses from the device
> > +		 * can't use this domain's ASID once the CD is clear.
> > +		 *
> > +		 * Sadly that doesn't apply to ATCs, which are PASID tagged.
> > +		 * Invalidate all other devices as well, because even though
> > +		 * they weren't 'officially' attached to the auxiliary domain,
> > +		 * they could have formed ATC entries.
> > +		 */
> > +		arm_smmu_atc_inv_domain(smmu_domain, 0, 0);
> 
> I've been struggling to understand the locking here, since both
> arm_smmu_write_ctx_desc and arm_smmu_atc_inv_domain take and release the
> devices_lock for the domain. Is there not a problem with devices coming and
> going in-between the two calls?

Yes, I need to think about this more. I bet there are plenty more issues
like this. For example I don't think I currently prevent the parent
domain from disappearing while auxiliary domains are attached.

> >  static struct iommu_ops arm_smmu_ops = {
> >  	.capable		= arm_smmu_capable,
> >  	.domain_alloc		= arm_smmu_domain_alloc,
> > @@ -2539,6 +2772,13 @@ static struct iommu_ops arm_smmu_ops = {
> >  	.of_xlate		= arm_smmu_of_xlate,
> >  	.get_resv_regions	= arm_smmu_get_resv_regions,
> >  	.put_resv_regions	= arm_smmu_put_resv_regions,
> > +	.dev_has_feat		= arm_smmu_dev_has_feature,
> > +	.dev_feat_enabled	= arm_smmu_dev_feature_enabled,
> > +	.dev_enable_feat	= arm_smmu_dev_enable_feature,
> > +	.dev_disable_feat	= arm_smmu_dev_disable_feature,
> 
> Why can't we use the existing ->capable and ->dev_{get,set}_attr callbacks
> for this?

->capable isn't very useful because it applies to all SMMUs in the
system. The existing ->{get,set}_attr callbacks apply to an
iommu_domain. I think the main reason for doing it on endpoints was that
it would be tedious to keep track of capabilities when attaching and
detaching devices to a domain, especially for drivers that allow
multiple IOMMUs per domain [1]. There were more discussions, and in the
end we agreed on this API for device attributes [2].

Thanks,
Jean

[1] https://lore.kernel.org/lkml/aa1ff748-c2ec-acc0-f1d9-cdff2b131e58@linux.intel.com/
[2] https://lore.kernel.org/linux-iommu/20181207102926.GM16835@8bytes.org/


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v7 3/4] ASoC: rockchip_max98090: Add dai_link for HDMI
From: Jernej Škrabec @ 2019-09-19 15:08 UTC (permalink / raw)
  To: Cheng-Yi Chiang
  Cc: alsa-devel, Heiko Stuebner, Neil Armstrong, David Airlie,
	dri-devel, linux-kernel, Hans Verkuil, Andrzej Hajda,
	Laurent Pinchart, Jerome Brunet, Takashi Iwai, linux-rockchip,
	dgreid, tzungbi, Jonas Karlman, Liam Girdwood, Russell King,
	Mark Brown, Jaroslav Kysela, linux-arm-kernel, dianders,
	Daniel Vetter
In-Reply-To: <20190919135450.62309-4-cychiang@chromium.org>

Hi!

Dne četrtek, 19. september 2019 ob 15:54:49 CEST je Cheng-Yi Chiang 
napisal(a):
> Use two dai_links. One for HDMI and one for max98090.
> With this setup, audio can play to speaker and HDMI selectively.
> 
> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
>  .../boot/dts/rk3288-veyron-analog-audio.dtsi  |   1 +
>  sound/soc/rockchip/rockchip_max98090.c        | 129 ++++++++++++++----
>  2 files changed, 103 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/rk3288-veyron-analog-audio.dtsi
> b/arch/arm/boot/dts/rk3288-veyron-analog-audio.dtsi index
> 445270aa136e..51208d161d65 100644
> --- a/arch/arm/boot/dts/rk3288-veyron-analog-audio.dtsi
> +++ b/arch/arm/boot/dts/rk3288-veyron-analog-audio.dtsi
> @@ -17,6 +17,7 @@
>  		rockchip,hp-det-gpios = <&gpio6 RK_PA5 
GPIO_ACTIVE_HIGH>;
>  		rockchip,mic-det-gpios = <&gpio6 RK_PB3 
GPIO_ACTIVE_LOW>;
>  		rockchip,headset-codec = <&headsetcodec>;
> +		rockchip,hdmi-codec = <&hdmi>;
>  	};
>  };
> 
> diff --git a/sound/soc/rockchip/rockchip_max98090.c
> b/sound/soc/rockchip/rockchip_max98090.c index c5fc24675a33..6c217492bb30
> 100644
> --- a/sound/soc/rockchip/rockchip_max98090.c
> +++ b/sound/soc/rockchip/rockchip_max98090.c
> @@ -11,6 +11,7 @@
>  #include <linux/gpio.h>
>  #include <linux/of_gpio.h>
>  #include <sound/core.h>
> +#include <sound/hdmi-codec.h>
>  #include <sound/jack.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
> @@ -41,6 +42,7 @@ static const struct snd_soc_dapm_widget rk_dapm_widgets[]
> = { SND_SOC_DAPM_MIC("Headset Mic", NULL),
>  	SND_SOC_DAPM_MIC("Int Mic", NULL),
>  	SND_SOC_DAPM_SPK("Speaker", NULL),
> +	SND_SOC_DAPM_LINE("HDMI", NULL),
>  };
> 
>  static const struct snd_soc_dapm_route rk_audio_map[] = {
> @@ -52,6 +54,7 @@ static const struct snd_soc_dapm_route rk_audio_map[] = {
>  	{"Headphone", NULL, "HPR"},
>  	{"Speaker", NULL, "SPKL"},
>  	{"Speaker", NULL, "SPKR"},
> +	{"HDMI", NULL, "TX"},
>  };
> 
>  static const struct snd_kcontrol_new rk_mc_controls[] = {
> @@ -59,6 +62,7 @@ static const struct snd_kcontrol_new rk_mc_controls[] = {
>  	SOC_DAPM_PIN_SWITCH("Headset Mic"),
>  	SOC_DAPM_PIN_SWITCH("Int Mic"),
>  	SOC_DAPM_PIN_SWITCH("Speaker"),
> +	SOC_DAPM_PIN_SWITCH("HDMI"),
>  };
> 
>  static int rk_aif1_hw_params(struct snd_pcm_substream *substream,
> @@ -92,38 +96,63 @@ static int rk_aif1_hw_params(struct snd_pcm_substream
> *substream,
> 
>  	ret = snd_soc_dai_set_sysclk(cpu_dai, 0, mclk,
>  				     SND_SOC_CLOCK_OUT);
> -	if (ret < 0) {
> -		dev_err(codec_dai->dev, "Can't set codec clock %d\n", 
ret);
> +	if (ret) {
> +		dev_err(cpu_dai->dev, "Can't set cpu dai clock %d\n", 
ret);
>  		return ret;
>  	}
> 
> +	/* HDMI codec dai does not need to set sysclk. */
> +	if (!strcmp(rtd->dai_link->name, "HDMI"))
> +		return 0;
> +
>  	ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
>  				     SND_SOC_CLOCK_IN);
> -	if (ret < 0) {
> -		dev_err(codec_dai->dev, "Can't set codec clock %d\n", 
ret);
> +	if (ret) {
> +		dev_err(codec_dai->dev, "Can't set codec dai clock 
%d\n", ret);
>  		return ret;
>  	}
> 
> -	return ret;
> +	return 0;
>  }
> 
>  static const struct snd_soc_ops rk_aif1_ops = {
>  	.hw_params = rk_aif1_hw_params,
>  };
> 
> -SND_SOC_DAILINK_DEFS(hifi,
> -	DAILINK_COMP_ARRAY(COMP_EMPTY()),
> -	DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "HiFi")),
> -	DAILINK_COMP_ARRAY(COMP_EMPTY()));
> -
> -static struct snd_soc_dai_link rk_dailink = {
> -	.name = "max98090",
> -	.stream_name = "Audio",
> -	.ops = &rk_aif1_ops,
> -	/* set max98090 as slave */
> -	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> -		SND_SOC_DAIFMT_CBS_CFS,
> -	SND_SOC_DAILINK_REG(hifi),
> +SND_SOC_DAILINK_DEFS(analog,
> +		     DAILINK_COMP_ARRAY(COMP_EMPTY()),
> +		     DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "HiFi")),
> +		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> +
> +SND_SOC_DAILINK_DEFS(hdmi,
> +		     DAILINK_COMP_ARRAY(COMP_EMPTY()),
> +		     DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "i2s-hifi")),
> +		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
> +
> +enum {
> +	DAILINK_MAX98090,
> +	DAILINK_HDMI,
> +};
> +
> +/* max98090 and HDMI codec dai_link */
> +static struct snd_soc_dai_link rk_dailinks[] = {
> +	[DAILINK_MAX98090] = {
> +		.name = "max98090",
> +		.stream_name = "Analog",
> +		.ops = &rk_aif1_ops,
> +		/* set max98090 as slave */
> +		.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> +			SND_SOC_DAIFMT_CBS_CFS,
> +		SND_SOC_DAILINK_REG(analog),
> +	},
> +	[DAILINK_HDMI] = {
> +		.name = "HDMI",
> +		.stream_name = "HDMI",
> +		.ops = &rk_aif1_ops,
> +		.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> +			SND_SOC_DAIFMT_CBS_CFS,
> +		SND_SOC_DAILINK_REG(hdmi),
> +	}
>  };
> 
>  static int rk_98090_headset_init(struct snd_soc_component *component);
> @@ -136,8 +165,8 @@ static struct snd_soc_aux_dev rk_98090_headset_dev = {
>  static struct snd_soc_card snd_soc_card_rk = {
>  	.name = "ROCKCHIP-I2S",
>  	.owner = THIS_MODULE,
> -	.dai_link = &rk_dailink,
> -	.num_links = 1,
> +	.dai_link = rk_dailinks,
> +	.num_links = ARRAY_SIZE(rk_dailinks),
>  	.aux_dev = &rk_98090_headset_dev,
>  	.num_aux_devs = 1,
>  	.dapm_widgets = rk_dapm_widgets,
> @@ -173,27 +202,73 @@ static int snd_rk_mc_probe(struct platform_device
> *pdev) int ret = 0;
>  	struct snd_soc_card *card = &snd_soc_card_rk;
>  	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *np_analog;
> +	struct device_node *np_cpu;
> +	struct device_node *np_hdmi_codec;
> +	struct of_phandle_args args;
> 
>  	/* register the soc card */
>  	card->dev = &pdev->dev;
> 
> -	rk_dailink.codecs->of_node = of_parse_phandle(np,
> -			"rockchip,audio-codec", 0);
> -	if (!rk_dailink.codecs->of_node) {
> +	np_analog = of_parse_phandle(np, "rockchip,audio-codec", 0);
> +	if (!np_analog) {
>  		dev_err(&pdev->dev,
>  			"Property 'rockchip,audio-codec' missing or 
invalid\n");
>  		return -EINVAL;
>  	}
> +	rk_dailinks[DAILINK_MAX98090].codecs->of_node = np_analog;
> +
> +	ret = of_parse_phandle_with_fixed_args(np, "rockchip,audio-codec",
> +					       0, 0, &args);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Unable to parse property 'rockchip,audio-
codec'\n");
> +		return ret;
> +	}
> +
> +	ret = snd_soc_get_dai_name(
> +			&args, &rk_dailinks[DAILINK_MAX98090].codecs-
>dai_name);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to get codec dai_name\n");
> +		return ret;
> +	}
> +
> +	np_cpu = of_parse_phandle(np, "rockchip,i2s-controller", 0);
> 
> -	rk_dailink.cpus->of_node = of_parse_phandle(np,
> -			"rockchip,i2s-controller", 0);
> -	if (!rk_dailink.cpus->of_node) {
> +	if (!np_cpu) {
>  		dev_err(&pdev->dev,
>  			"Property 'rockchip,i2s-controller' missing 
or invalid\n");
>  		return -EINVAL;
>  	}
> 
> -	rk_dailink.platforms->of_node = rk_dailink.cpus->of_node;
> +	np_hdmi_codec = of_parse_phandle(np, "rockchip,hdmi-codec", 0);
> +	if (!np_hdmi_codec) {
> +		dev_err(&pdev->dev,
> +			"Property 'rockchip,hdmi-codec' missing or 
invalid\n");
> +		return -EINVAL;
> +	}

Property "rockchip,hdmi-codec" is added in this series, right? You can't make 
it mandatory, because kernel must be backward compatible with old device tree 
files and they don't have this property.

Think about use case when user happily used this driver and after kernel 
update, it suddenly stops working. You can't assume that board DTB file will be 
updated along with kernel update.

Just make it optional and don't expose jack functionality if it's not present.

Best regards,
Jernej

> +
> +	rk_dailinks[DAILINK_HDMI].codecs->of_node = np_hdmi_codec;
> +
> +	ret = of_parse_phandle_with_fixed_args(np, "rockchip,hdmi-codec",
> +					       0, 0, &args);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Unable to parse property 'rockchip,hdmi-
codec'\n");
> +		return ret;
> +	}
> +
> +	ret = snd_soc_get_dai_name(
> +			&args, &rk_dailinks[DAILINK_HDMI].codecs-
>dai_name);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to get hdmi codec 
dai_name\n");
> +		return ret;
> +	}
> +
> +	rk_dailinks[DAILINK_MAX98090].cpus->of_node = np_cpu;
> +	rk_dailinks[DAILINK_MAX98090].platforms->of_node = np_cpu;
> +	rk_dailinks[DAILINK_HDMI].cpus->of_node = np_cpu;
> +	rk_dailinks[DAILINK_HDMI].platforms->of_node = np_cpu;
> 
>  	rk_98090_headset_dev.codec_of_node = of_parse_phandle(np,
>  			"rockchip,headset-codec", 0);





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 8/8] iommu/arm-smmu-v3: Add support for PCI PASID
From: Jean-Philippe Brucker @ 2019-09-19 15:10 UTC (permalink / raw)
  To: Auger Eric
  Cc: mark.rutland, devicetree, jacob.jun.pan, Jean-Philippe Brucker,
	joro, will.deacon, linux-kernel, iommu, robh+dt, robin.murphy,
	linux-arm-kernel
In-Reply-To: <b0e3d9a9-6085-b393-1982-3dd95bf5d100@redhat.com>

On Mon, Jul 08, 2019 at 09:58:16AM +0200, Auger Eric wrote:
> > +	ret = pci_enable_pasid(pdev, features);
> > +	if (!ret)
> > +		master->ssid_bits = min_t(u8, ilog2(num_pasids),
> > +					  master->smmu->ssid_bits);
> I don't really get why this setting is conditional to the success of
> pci_enabled_pasid and not num_pasids > 0.

num_pasids only contains the value of the PCIe PASID capability. If
pci_enable_pasid() fails then we want to leave master->ssid_bits to 0 so
that we report to users that SVA and AUXD aren't supported.

> If it fails the ssid_bits is set to min(smmu->ssid_bits,
> fwspec->num_pasid_bits) anyway.
>
> > +	return ret;
> > +}
> > +
> > +static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
> > +{
> > +	struct pci_dev *pdev;
> > +
> > +	if (!dev_is_pci(master->dev))
> > +		return;
> > +
> > +	pdev = to_pci_dev(master->dev);
> > +
> > +	if (!pdev->pasid_enabled)
> > +		return;
> > +
> > +	pci_disable_pasid(pdev);
> > +	master->ssid_bits = 0;
> in case of a platform device you leave the ssid_bits to a value != 0. Is
> that what you want?

Yes, this is only for PCI devices, there is no standard way of disabling
PASID in platform devices. We just take whatever the firmware gives us.

> > +}
> > +
> >  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
> >  {
> >  	unsigned long flags;
> > @@ -2413,6 +2456,9 @@ static int arm_smmu_add_device(struct device *dev)
> >  
> >  	master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits);
> >  
> > +	/* Note that PASID must be enabled before, and disabled after ATS */
> > +	arm_smmu_enable_pasid(master);
> In case the call fails, don't you want to handle the error and reset the
> ssid_bits?

This function fails if the device doesn't support PASID, and we leave
ssid_bits to 0. That said, I think it would be nicer to move the above
line (that deals with fwspec) into arm_smmu_enable_pasid()

Thanks,
Jean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 1/1] sched/rt: avoid contend with CFS task
From: Qais Yousef @ 2019-09-19 15:11 UTC (permalink / raw)
  To: Vincent Guittot
  Cc: wsd_upstream, Peter Zijlstra, linux-kernel, Jing-Ting Wu,
	linux-mediatek, Matthias Brugger, Valentin Schneider, LAK
In-Reply-To: <CAKfTPtAy1JSh725GAVXmg_x3fby1UfYn504tq4n2rQs1-JMy6Q@mail.gmail.com>

On 09/19/19 16:37, Vincent Guittot wrote:
> On Thu, 19 Sep 2019 at 16:32, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Thu, 19 Sep 2019 at 16:23, Qais Yousef <qais.yousef@arm.com> wrote:
> > >
> > > On 09/19/19 14:27, Vincent Guittot wrote:
> > > > > > > But for requirement of performance, I think it is better to differentiate between idle CPU and CPU has CFS task.
> > > > > > >
> > > > > > > For example, we use rt-app to evaluate runnable time on non-patched environment.
> > > > > > > There are (NR_CPUS-1) heavy CFS tasks and 1 RT Task. When a CFS task is running, the RT task wakes up and choose the same CPU.
> > > > > > > The CFS task will be preempted and keep runnable until it is migrated to another cpu by load balance.
> > > > > > > But load balance is not triggered immediately, it will be triggered until timer tick hits with some condition satisfied(ex. rq->next_balance).
> > > > > >
> > > > > > Yes you will have to wait for the next tick that will trigger an idle
> > > > > > load balance because you have an idle cpu and 2 runnable tack (1 RT +
> > > > > > 1CFS) on the same CPU. But you should not wait for more than  1 tick
> > > > > >
> > > > > > The current load_balance doesn't handle correctly the situation of 1
> > > > > > CFS and 1 RT task on same CPU while 1 CPU is idle. There is a rework
> > > > > > of the load_balance that is under review on the mailing list that
> > > > > > fixes this problem and your CFS task should migrate to the idle CPU
> > > > > > faster than now
> > > > > >
> > > > >
> > > > > Period load balance should be triggered when current jiffies is behind
> > > > > rq->next_balance, but rq->next_balance is not often exactly the same
> > > > > with next tick.
> > > > > If cpu_busy, interval = sd->balance_interval * sd->busy_factor, and
> > > >
> > > > But if there is an idle CPU on the system, the next idle load balance
> > > > should apply shortly because the busy_factor is not used for this CPU
> > > > which is  not busy.
> > > > In this case, the next_balance interval is sd_weight which is probably
> > > > 4ms at cluster level and 8ms at system level in your case. This means
> > > > between 1 and 2 ticks
> > >
> > > But if the CFS task we're preempting was latency sensitive - this 1 or 2 tick
> > > is too late of a recovery.
> > >
> > > So while it's good we recover, but a preventative approach would be useful too.
> > > Just saying :-) I'm still not sure if this is the best longer term approach.
> >
> > like using a rt task ?
> 
> I mean, RT task should select a sub optimal CPU because of CFS
> If you want to favor CFS compared to RT it's probably because your
> task should be RT too

Yes possibly. But I don't think this is always doable. Especially when you're
running on generic system not a special purposed one.

And we don't need to favor CFS over RT. But I think they can play nicely
together.

For example on Android there are few RT tasks and rarely more than 1 runnable
RT task at a time. But if it happened to wakeup on the same CPU that is
running the UI thread you could lose a frame. And from what I've seen as well
we have 1-3 CFS tasks runnable, weighted more towards 1 task. So we do have
plenty of idle CPUs on average.

But as I mentioned earlier I couldn't prove yet this being a serious problem.
I was hoping the use case presented here is based on a real workload, but it's
synthetic. So I agree we need stronger reasons, but I think conceptually we do
have a conflict of interest where RT task could unnecessarily hurt the
performance of CFS task.

Another way to look at the problem is that the system is not partitioned
correctly and the admin could do a better job to prevent this.

--
Qais Yousef

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH RFC 11/14] arm64: Move the ASID allocator code in a separate file
From: Jean-Philippe Brucker @ 2019-09-19 15:18 UTC (permalink / raw)
  To: Guo Ren
  Cc: aou, Linux Kernel Mailing List, Arnd Bergmann, suzuki.poulose,
	Marc Zyngier, Catalin Marinas, Palmer Dabbelt, christoffer.dall,
	iommu, Mike Rapoport, Anup Patel, Atish Patra, Julien Grall,
	james.morse, gary, Paul Walmsley, linux-riscv, Will Deacon,
	kvmarm, linux-arm-kernel
In-Reply-To: <CAJF2gTRbyfrUqAULPqJTXdxx8YOscPqAEuMsoJ+dTNobNrUV1g@mail.gmail.com>

On Thu, Sep 19, 2019 at 09:07:15PM +0800, Guo Ren wrote:
> > The solution I had to this problem is pinning the ASID [1] used by the
> > IOMMU, to prevent the CPU from recycling the ASID on rollover. This way
> > the CPU doesn't have to wait for IOMMU invalidations to complete, when
> > scheduling a task that might not even have anything to do with the IOMMU.
> >
> 
> > In the Arm SMMU, ASID and IOASID (PASID) are separate identifiers. IOASID
> > indexes an entry in the context descriptor table, which contains the ASID.
> > So with unpinned shared ASID you don't need to invalidate the ATC on
> > rollover, since the IOASID doesn't change, but you do need to modify the
> > context descriptor and invalidate cached versions of it.
> The terminology confused me a lot. I perfer use PASID for IOMMU and
> ASID is for CPU.
> Arm's entry of the context descriptor table contains a "IOASID"

The terminology I've been using so far is different:
* IOASID is PASID
* The entry in the context descriptor table contains an ASID, which
  is either "shared" with CPUs or "private" to the SMMU (the SMMU spec
  says "shared" or "non-shared").
* So the CPU and SMMU TLBs use ASIDs, and the PCI ATC uses IOASID

> IOASID != ASID for CPU_TLB and IOMMU_TLB.
> 
> When you say "since the IOASID doesn't change",Is it PASID or my IOASID ? -_*!

I was talking about PASID. Maybe we can drop "IOASID" and talk only
about ASID and PASID :)

> PASID in PCI-sig was used to determine transfer address space.
> For intel, the entry which is indexed by PASID also contain S1/S2.PGD
> and DID(VMID).
> For arm, the entry which is indexed by PASID only contain S1.PGD and
> IOASID. Compare to Intel Vt-d Scalable mode, arm's design can't
> support PCI Virtual Function.

The SMMU does support PCI Virtual Function - an hypervisor can assign a
VF to a guest, and let that guest partition the VF into smaller contexts
by using PASID.  What it can't support is assigning partitions of a PCI
function (VF or PF) to multiple Virtual Machines, since there is a
single S2 PGD per function (in the Stream Table Entry), rather than one
S2 PGD per PASID context.

Thanks,
Jean

> > Once you have pinned ASIDs, you could also declare that IOASID = ASID. I
> > don't remember finding an argument to strictly forbid it, even though ASID
> > and IOASID have different sizes on Arm (respectively 8/16 and 20 bits).
> ASID and IOASID are hard to keep the same between CPU system and IOMMU
> system. So I introduce S1/S2.PGD.PPN as a bridge between CPUs and
> IOMMUs.
> See my proposal [1]
> 
> 1: https://lore.kernel.org/linux-csky/1568896556-28769-1-git-send-email-guoren@kernel.org/T/#u
> -- 
> Best Regards
>  Guo Ren
> 
> ML: https://lore.kernel.org/linux-csky/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v4 2/3] dmaengine: imx-sdma: fix dma freezes
From: Jan Lübbe @ 2019-09-19 15:19 UTC (permalink / raw)
  To: Philipp Puschmann, linux-kernel
  Cc: fugang.duan, shawnguo, s.hauer, vkoul, linux-imx, kernel,
	dan.j.williams, yibin.gong, festevam, dmaengine, linux-arm-kernel,
	l.stach
In-Reply-To: <20190919142942.12469-3-philipp.puschmann@emlix.com>

[-- Attachment #1: Type: text/plain, Size: 3663 bytes --]

Hi Philipp,

see below...

On Thu, 2019-09-19 at 16:29 +0200, Philipp Puschmann wrote:
> For some years and since many kernel versions there are reports that the
> RX UART SDMA channel stops working at some point. The workaround was to
> disable DMA for RX. This commit tries to fix the problem itself.
> 
> Due to its license i wasn't able to debug the sdma script itself but it
> somehow leads to blocking the scheduling of the channel script when a
> running sdma script does not find any free descriptor in the ring to put
> its data into.
> 
> If we detect such a potential case we manually restart the channel.
> 
> As sdmac->desc is constant we can move desc out of the loop.
> 
> Fixes: 1ec1e82f2510 ("dmaengine: Add Freescale i.MX SDMA support")
> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> 
> Changelog v4:
>  - fixed the fixes tag
>  
> Changelog v3:
>  - use correct dma_wmb() instead of dma_wb()
>  - add fixes tag
>  
> Changelog v2:
>  - clarify comment and commit description
> 
>  drivers/dma/imx-sdma.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index e029a2443cfc..a32b5962630e 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -775,21 +775,23 @@ static void sdma_start_desc(struct sdma_channel *sdmac)
>  static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  {
>  	struct sdma_buffer_descriptor *bd;
> -	int error = 0;
> -	enum dma_status	old_status = sdmac->status;
> +	struct sdma_desc *desc = sdmac->desc;
> +	int error = 0, cnt = 0;
> +	enum dma_status old_status = sdmac->status;
>  
>  	/*
>  	 * loop mode. Iterate over descriptors, re-setup them and
>  	 * call callback function.
>  	 */
> -	while (sdmac->desc) {
> -		struct sdma_desc *desc = sdmac->desc;
> +	while (desc) {
>  
>  		bd = &desc->bd[desc->buf_tail];
>  
>  		if (bd->mode.status & BD_DONE)
>  			break;
>  
> +		cnt++;
> +
>  		if (bd->mode.status & BD_RROR) {
>  			bd->mode.status &= ~BD_RROR;
>  			sdmac->status = DMA_ERROR;
> @@ -822,6 +824,17 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  		if (error)
>  			sdmac->status = old_status;
>  	}
> +
> +	/* In some situations it may happen that the sdma does not found any
                                                          ^ hasn't
> +	 * usable descriptor in the ring to put data into. The channel is
> +	 * stopped then. While there is no specific error condition we can
> +	 * check for, a necessary condition is that all available buffers for
> +	 * the current channel have been written to by the sdma script. In
> +	 * this case and after we have made the buffers available again,
> +	 * we restart the channel.
> +	 */

Are you sure we can't miss cases where we only had to make some buffers
available again, but the SDMA already ran out of buffers before?

A while ago, I was debugging a similar issue triggered by receiving
data with a wrong baud rate, which leads to all descriptors being
marked with the error flag very quickly (and the SDMA stalling).
I noticed that you can check if the channel is still running by
checking the SDMA_H_STATSTOP register & BIT(sdmac->channel).

I also added a flag for the sdmac->flags field to allow stopping the
channel from the callback (otherwise it would enable the channel
again).

Attached is my current version of that patch for reference.

> +	if (cnt >= desc->num_bd)
> +		sdma_enable_channel(sdmac->sdma, sdmac->channel);
>  }
>  
>  static void mxc_sdma_handle_channel_normal(struct sdma_channel *data)

[-- Attachment #2: 0001-dmaengine-imx-sdma-restart-stopped-cyclic-transfers.patch --]
[-- Type: text/x-patch, Size: 2949 bytes --]

From 73d7dcf84dac5512c50448ff6adf084f1a9bd6f9 Mon Sep 17 00:00:00 2001
From: Jan Luebbe <jlu@pengutronix.de>
Date: Tue, 16 Apr 2019 18:35:04 +0200
Subject: [PATCH] dmaengine: imx-sdma: restart stopped cyclic transfers

For cyclic DMA transfers, we have at least two cases where we can run
out descriptors available to the engine:
- Interrups are disabled for too long and all buffers a filled with
  data.
- DMA errors (such as generated by baud rate mismatch with imx-uart) use
  up all descriptors before we can react.

In this case, SDMA stops the channel and no further transfers are done
until the respective channel is disabled and re-enabled.

The best we can do in this case is to check if the transfer should still
be enabled (it could have been disabled during
sdma_update_channel_loop), but the SDMA channel is stopped. In this
case, we re-start the channel.

To avoid racing with changes to the sdmac->status field (which is
written and restored in sdma_update_channel_loop), we add a new flag
(IMX_DMA_ACTIVE) to indicate that the channel is currently active.

Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 58fa8520892b..8774259af24c 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -383,6 +383,7 @@ struct sdma_channel {
 };
 
 #define IMX_DMA_SG_LOOP		BIT(0)
+#define IMX_DMA_ACTIVE		BIT(1)
 
 #define MAX_DMA_CHANNELS 32
 #define MXC_SDMA_DEFAULT_PRIORITY 1
@@ -658,6 +659,9 @@ static int sdma_config_ownership(struct sdma_channel *sdmac,
 
 static void sdma_enable_channel(struct sdma_engine *sdma, int channel)
 {
+	struct sdma_channel *sdmac = &sdma->channel[channel];
+
+	sdmac->flags |= IMX_DMA_ACTIVE;
 	writel(BIT(channel), sdma->regs + SDMA_H_START);
 }
 
@@ -774,6 +778,7 @@ static void sdma_start_desc(struct sdma_channel *sdmac)
 
 static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 {
+	struct sdma_engine *sdma = sdmac->sdma;
 	struct sdma_buffer_descriptor *bd;
 	int error = 0;
 	enum dma_status	old_status = sdmac->status;
@@ -820,6 +825,13 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		if (error)
 			sdmac->status = old_status;
 	}
+
+	if ((sdmac->flags & IMX_DMA_ACTIVE) &&
+	    !(readl_relaxed(sdma->regs + SDMA_H_STATSTOP) & BIT(sdmac->channel))) {
+		dev_err_ratelimited(sdma->dev, "SDMA channel %d: cyclic transfer disabled by HW, reenabling\n",
+				sdmac->channel);
+		writel(BIT(sdmac->channel), sdma->regs + SDMA_H_START);
+	};
 }
 
 static void mxc_sdma_handle_channel_normal(struct sdma_channel *data)
@@ -1049,6 +1061,7 @@ static int sdma_disable_channel(struct dma_chan *chan)
 	struct sdma_engine *sdma = sdmac->sdma;
 	int channel = sdmac->channel;
 
+	sdmac->flags &= ~IMX_DMA_ACTIVE;
 	writel_relaxed(BIT(channel), sdma->regs + SDMA_H_STATSTOP);
 	sdmac->status = DMA_ERROR;
 
-- 
2.23.0


[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* Re: [EXT] Re: SCMI & Devfreq
From: Sudeep Holla @ 2019-09-19 15:23 UTC (permalink / raw)
  To: Sujeet Kumar Baranwal
  Cc: linux-pm, linux-arm-kernel@lists.infradead.org, Sudeep Holla
In-Reply-To: <BYAPR18MB2438047B622951C6EFE92FABAF8E0@BYAPR18MB2438.namprd18.prod.outlook.com>

(Adding linux-pm list)

On Wed, Sep 18, 2019 at 10:53:07PM +0000, Sujeet Kumar Baranwal wrote:
> Sudeep, One trivial question wrt SCMI-CPUFREQ framework.
> 
> The SCMI perf protocol would tell what are different frequencies the
> platform support in the beginning.
>
> For example, the command :
> cat  /sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies
> shows:
> 280000 560000 840000 1120000 1400000 1820000 1960000 2240000 2520000 2800000
>
> /* Attempt to change the frequency */
> ~ # echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
> It works.
>
> But in a scenario, where user tries a number which is not listed, SCP
> applies its own logic to get a nearby value frequency for CPU and returns.
>
> My question is that could we add some print message in kernel that user wish
> is not exactly fulfilled, an approximation has been done so the user
> explicitly knows his command has been partially met.  If this to happen, a
> patch might be needed in kernel. What is your opinion?
>

May be, you need to check with the maintainers ? :)

The path of execution is:
cpufreq_set(policy, freq) [cpufreq_userspace.c]
__cpufreq_driver_target(policy, freq, CPUFREQ_RELATION_L) [cpufreq_userspace.c]
index = cpufreq_frequency_table_target(policy, target_freq, relation) [cpufreq.c]
__target_index(policy, index) [cpufreq.c]

So if you need logs, it needs to be in core file rather than individual
drivers.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH 0/2] Add Support for MMC/SD for J721e-base-board
From: Faiz Abbas @ 2019-09-19 15:32 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel
  Cc: mark.rutland, nm, robh+dt, t-kristo

The following are dts patches to add MMC/SD Support on TI's J721e base
board.

Patches depend on Lokesh's gpio patches[1] and device exclusivity patches[2].

[1] https://patchwork.kernel.org/cover/11085643/
[2] https://patchwork.kernel.org/cover/11051559/

Faiz Abbas (2):
  arm64: dts: ti: j721e-main: Add SDHCI nodes
  arm64: dts: ti: j721e-common-proc-board: Add Support for eMMC and SD
    card

 .../dts/ti/k3-j721e-common-proc-board.dts     | 34 +++++++++++++
 arch/arm64/boot/dts/ti/k3-j721e-main.dtsi     | 50 +++++++++++++++++++
 2 files changed, 84 insertions(+)

-- 
2.19.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH 1/2] arm64: dts: ti: j721e-main: Add SDHCI nodes
From: Faiz Abbas @ 2019-09-19 15:32 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel
  Cc: mark.rutland, nm, robh+dt, t-kristo
In-Reply-To: <20190919153242.29399-1-faiz_abbas@ti.com>

Add nodes for the 3 SDHCI instances present on TI's J721E device.
instance 0 supports HS400 (8 bit bus widht, DDR, 400 MBps)
while instances 1 and 2 support SDR104 (4 bit width, SDR, 100 MBps) as
their highest speed modes. Currently, only High speed (50 MHz clock) has
been enabled.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 50 +++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
index 199bc9a00b20..1650bbd10932 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
@@ -372,4 +372,54 @@
 		clocks = <&k3_clks 112 0>;
 		clock-names = "gpio";
 	};
+
+	main_sdhci0: sdhci@4f80000 {
+		compatible = "ti,j721e-sdhci-8bit";
+		reg = <0x0 0x4f80000 0x0 0x1000>, <0x0 0x4f88000 0x0 0x400>;
+		interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
+		power-domains = <&k3_pds 91 TI_SCI_PD_EXCLUSIVE>;
+		clock-names = "clk_xin", "clk_ahb";
+		clocks = <&k3_clks 91 1>, <&k3_clks 91 0>;
+		assigned-clocks = <&k3_clks 91 1>;
+		assigned-clock-parents = <&k3_clks 91 2>;
+		bus-width = <8>;
+		mmc-hs400-1_8v;
+		mmc-ddr-1_8v;
+		ti,otap-del-sel = <0x2>;
+		ti,trm-icp = <0x8>;
+		ti,strobe-sel = <0x77>;
+		dma-coherent;
+	};
+
+	main_sdhci1: sdhci@4fb0000 {
+		compatible = "ti,j721e-sdhci-4bit";
+		reg = <0x0 0x04fb0000 0x0 0x1000>, <0x0 0x4fb8000 0x0 0x400>;
+		interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+		power-domains = <&k3_pds 92 TI_SCI_PD_EXCLUSIVE>;
+		clock-names = "clk_xin", "clk_ahb";
+		clocks = <&k3_clks 92 0>, <&k3_clks 92 5>;
+		assigned-clocks = <&k3_clks 92 0>;
+		assigned-clock-parents = <&k3_clks 92 1>;
+		ti,otap-del-sel = <0x2>;
+		ti,trm-icp = <0x8>;
+		ti,clkbuf-sel = <0x7>;
+		dma-coherent;
+		no-1-8-v;
+	};
+
+	main_sdhci2: sdhci@4f98000 {
+		compatible = "ti,j721e-sdhci-4bit";
+		reg = <0x0 0x4f98000 0x0 0x1000>, <0x0 0x4f90000 0x0 0x400>;
+		interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+		power-domains = <&k3_pds 93 TI_SCI_PD_EXCLUSIVE>;
+		clock-names = "clk_xin", "clk_ahb";
+		clocks = <&k3_clks 93 0>, <&k3_clks 93 5>;
+		assigned-clocks = <&k3_clks 93 0>;
+		assigned-clock-parents = <&k3_clks 93 1>;
+		ti,otap-del-sel = <0x2>;
+		ti,trm-icp = <0x8>;
+		ti,clkbuf-sel = <0x7>;
+		dma-coherent;
+		no-1-8-v;
+	};
 };
-- 
2.19.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 2/2] arm64: dts: ti: j721e-common-proc-board: Add Support for eMMC and SD card
From: Faiz Abbas @ 2019-09-19 15:32 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel
  Cc: mark.rutland, nm, robh+dt, t-kristo
In-Reply-To: <20190919153242.29399-1-faiz_abbas@ti.com>

sdhci0 is connected to an eMMC and sdhci1 is connected to an SD card
slot. Add support for these nodes.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 .../dts/ti/k3-j721e-common-proc-board.dts     | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
index d2894d55fbbe..3cfaa2c83ba6 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
@@ -41,6 +41,20 @@
 			J721E_IOPAD(0x0, PIN_INPUT, 7) /* (AC18) EXTINTn.GPIO0_0 */
 		>;
 	};
+
+	main_mmc1_pins_default: main_mmc1_pins_default {
+		pinctrl-single,pins = <
+			J721E_IOPAD(0x254, PIN_INPUT, 0) /* (R29) MMC1_CMD */
+			J721E_IOPAD(0x250, PIN_INPUT, 0) /* (P25) MMC1_CLK */
+			J721E_IOPAD(0x2ac, PIN_INPUT, 0) /* (P25) MMC1_CLKLB */
+			J721E_IOPAD(0x24c, PIN_INPUT, 0) /* (R24) MMC1_DAT0 */
+			J721E_IOPAD(0x248, PIN_INPUT, 0) /* (P24) MMC1_DAT1 */
+			J721E_IOPAD(0x244, PIN_INPUT, 0) /* (R25) MMC1_DAT2 */
+			J721E_IOPAD(0x240, PIN_INPUT, 0) /* (R26) MMC1_DAT3 */
+			J721E_IOPAD(0x258, PIN_INPUT, 0) /* (P23) MMC1_SDCD */
+			J721E_IOPAD(0x25c, PIN_INPUT, 0) /* (R28) MMC1_SDWP */
+		>;
+	};
 };
 
 &wkup_pmx0 {
@@ -117,3 +131,23 @@
 &wkup_gpio1 {
 	status = "disabled";
 };
+
+&main_sdhci0 {
+	/* eMMC */
+	non-removable;
+	ti,driver-strength-ohm = <50>;
+	disable-wp;
+};
+
+&main_sdhci1 {
+	/* SD/MMC */
+	pinctrl-names = "default";
+	pinctrl-0 = <&main_mmc1_pins_default>;
+	ti,driver-strength-ohm = <50>;
+	disable-wp;
+};
+
+&main_sdhci2 {
+	/* Unused */
+	status = "disabled";
+};
-- 
2.19.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH] ASoC: xlnx: Use devm_platform_ioremap_resource() in xlnx_formatter_pcm_probe()
From: Markus Elfring @ 2019-09-19 15:38 UTC (permalink / raw)
  To: alsa-devel, linux-arm-kernel, Gustavo A. R. Silva,
	Jaroslav Kysela, Liam Girdwood, Mark Brown,
	Maruthi Srinivas Bayyavarapu, Michal Simek, Stephen Boyd,
	Stephen Rothwell, Takashi Iwai
  Cc: kernel-janitors, LKML

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 19 Sep 2019 17:27:57 +0200

Simplify this function implementation by using a known wrapper function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/soc/xilinx/xlnx_formatter_pcm.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/sound/soc/xilinx/xlnx_formatter_pcm.c b/sound/soc/xilinx/xlnx_formatter_pcm.c
index 48970efe7838..fb652e73abeb 100644
--- a/sound/soc/xilinx/xlnx_formatter_pcm.c
+++ b/sound/soc/xilinx/xlnx_formatter_pcm.c
@@ -564,7 +564,6 @@ static int xlnx_formatter_pcm_probe(struct platform_device *pdev)
 	int ret;
 	u32 val;
 	struct xlnx_pcm_drv_data *aud_drv_data;
-	struct resource *res;
 	struct device *dev = &pdev->dev;

 	aud_drv_data = devm_kzalloc(dev, sizeof(*aud_drv_data), GFP_KERNEL);
@@ -584,13 +583,7 @@ static int xlnx_formatter_pcm_probe(struct platform_device *pdev)
 		return ret;
 	}

-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(dev, "audio formatter node:addr to resource failed\n");
-		ret = -ENXIO;
-		goto clk_err;
-	}
-	aud_drv_data->mmio = devm_ioremap_resource(dev, res);
+	aud_drv_data->mmio = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(aud_drv_data->mmio)) {
 		dev_err(dev, "audio formatter ioremap failed\n");
 		ret = PTR_ERR(aud_drv_data->mmio);
--
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH] spi: atmel: Fix crash when using more than 4 gpio CS
From: Gregory CLEMENT @ 2019-09-19 15:38 UTC (permalink / raw)
  To: Mark Brown, linux-spi, linux-kernel, Nicolas Ferre
  Cc: Alexandre Belloni, Gregory CLEMENT, stable, Ludovic Desroches,
	Thomas Petazzoni, linux-arm-kernel

Even when using a gpio as chip select, the registers controlling the
hardware chip select have to be configured. However any of the 4
hardware CS can be controlled.

Until now, the gpio index was used to match an hardware chip select,
which limited the number of CS than can be used. Using more than 4
gpios leads to the following kernel crash:

WARNING: CPU: 0 PID: 1 at drivers/spi/spi-atmel.c:1293 atmel_spi_transfer_one_message+0x8b0/0x8ec
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 5.3.0+ #24
Hardware name: Atmel SAMA5
[<c010d914>] (unwind_backtrace) from [<c010ad84>] (show_stack+0x10/0x14)
[<c010ad84>] (show_stack) from [<c0117d28>] (__warn+0xd8/0xf0)
[<c0117d28>] (__warn) from [<c0117d80>] (warn_slowpath_null+0x40/0x48)
[<c0117d80>] (warn_slowpath_null) from [<c04b21dc>] (atmel_spi_transfer_one_message+0x8b0/0x8ec)
[<c04b21dc>] (atmel_spi_transfer_one_message) from [<c04afb20>] (__spi_pump_messages+0x330/0x510)
[<c04afb20>] (__spi_pump_messages) from [<c04afe5c>] (__spi_sync+0x150/0x158)
[<c04afe5c>] (__spi_sync) from [<c04afe88>] (spi_sync+0x24/0x3c)
[<c04afe88>] (spi_sync) from [<c04aff88>] (spi_write_then_read+0xe8/0x1a8)
[<c04aff88>] (spi_write_then_read) from [<c0454da0>] (_regmap_raw_read+0xec/0x148)
[<c0454da0>] (_regmap_raw_read) from [<c0454f6c>] (regmap_raw_read+0x110/0x238)
[<c0454f6c>] (regmap_raw_read) from [<c04551f4>] (regmap_bulk_read+0x160/0x19c)
[<c04551f4>] (regmap_bulk_read) from [<c0517744>] (rv3029_get_sr+0x28/0x64)
[<c0517744>] (rv3029_get_sr) from [<c0518184>] (rv3029_probe+0x5c/0x260)
[<c0518184>] (rv3029_probe) from [<c04acf90>] (spi_drv_probe+0x88/0xac)
[<c04acf90>] (spi_drv_probe) from [<c043fec8>] (really_probe+0xf0/0x2c8)
[<c043fec8>] (really_probe) from [<c0440214>] (driver_probe_device+0x60/0x16c)
[<c0440214>] (driver_probe_device) from [<c04404c0>] (device_driver_attach+0x58/0x60)
[<c04404c0>] (device_driver_attach) from [<c0440520>] (__driver_attach+0x58/0xcc)
[<c0440520>] (__driver_attach) from [<c043e318>] (bus_for_each_dev+0x78/0xc0)
[<c043e318>] (bus_for_each_dev) from [<c043f33c>] (bus_add_driver+0x15c/0x1e0)
[<c043f33c>] (bus_add_driver) from [<c0440d2c>] (driver_register+0x74/0x108)
[<c0440d2c>] (driver_register) from [<c0a1cb44>] (rv30x9_init+0x38/0x68)
[<c0a1cb44>] (rv30x9_init) from [<c0102608>] (do_one_initcall+0x58/0x1c0)
[<c0102608>] (do_one_initcall) from [<c0a00e88>] (kernel_init_freeable+0x124/0x1c0)
[<c0a00e88>] (kernel_init_freeable) from [<c0774dc0>] (kernel_init+0x8/0x10c)
[<c0774dc0>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
Exception stack(0xcf429fb0 to 0xcf429ff8)
9fa0:                                     00000000 00000000 00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000

With this patch, when using a gpio CS, the hardware CS0 is always
used. Thanks to this, there is no more limitation for the number of
gpio CS we can use.

Fixes: 754ce4f29937 ("[PATCH] SPI: atmel_spi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/spi/spi-atmel.c | 44 ++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index acf318e7330c..bb94f5927819 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -353,22 +353,28 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
 {
 	struct atmel_spi_device *asd = spi->controller_state;
 	u32 mr;
+	u8 chip_select;
+
+	if (as->use_cs_gpios)
+		chip_select = 0;
+	else
+		chip_select = spi->chip_select;
 
 	if (atmel_spi_is_v2(as)) {
-		spi_writel(as, CSR0 + 4 * spi->chip_select, asd->csr);
+		spi_writel(as, CSR0 + 4 * chip_select, asd->csr);
 		/* For the low SPI version, there is a issue that PDC transfer
 		 * on CS1,2,3 needs SPI_CSR0.BITS config as SPI_CSR1,2,3.BITS
 		 */
 		spi_writel(as, CSR0, asd->csr);
 		if (as->caps.has_wdrbt) {
 			spi_writel(as, MR,
-					SPI_BF(PCS, ~(0x01 << spi->chip_select))
+					SPI_BF(PCS, ~(0x01 << chip_select))
 					| SPI_BIT(WDRBT)
 					| SPI_BIT(MODFDIS)
 					| SPI_BIT(MSTR));
 		} else {
 			spi_writel(as, MR,
-					SPI_BF(PCS, ~(0x01 << spi->chip_select))
+					SPI_BF(PCS, ~(0x01 << chip_select))
 					| SPI_BIT(MODFDIS)
 					| SPI_BIT(MSTR));
 		}
@@ -390,8 +396,8 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
 		}
 
 		mr = spi_readl(as, MR);
-		mr = SPI_BFINS(PCS, ~(1 << spi->chip_select), mr);
-		if (as->use_cs_gpios && spi->chip_select != 0)
+		mr = SPI_BFINS(PCS, ~(1 << chip_select), mr);
+		if (as->use_cs_gpios && chip_select != 0)
 			gpiod_set_value(asd->npcs_pin, 1);
 		spi_writel(as, MR, mr);
 	}
@@ -403,12 +409,18 @@ static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi)
 {
 	struct atmel_spi_device *asd = spi->controller_state;
 	u32 mr;
+	u8 chip_select;
+
+	if (as->use_cs_gpios)
+		chip_select = 0;
+	else
+		chip_select = spi->chip_select;
 
 	/* only deactivate *this* device; sometimes transfers to
 	 * another device may be active when this routine is called.
 	 */
 	mr = spi_readl(as, MR);
-	if (~SPI_BFEXT(PCS, mr) & (1 << spi->chip_select)) {
+	if (~SPI_BFEXT(PCS, mr) & (1 << chip_select)) {
 		mr = SPI_BFINS(PCS, 0xf, mr);
 		spi_writel(as, MR, mr);
 	}
@@ -417,7 +429,7 @@ static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi)
 
 	if (!as->use_cs_gpios)
 		spi_writel(as, CR, SPI_BIT(LASTXFER));
-	else if (atmel_spi_is_v2(as) || spi->chip_select != 0)
+	else if (atmel_spi_is_v2(as) || chip_select != 0)
 		gpiod_set_value(asd->npcs_pin, 0);
 }
 
@@ -844,6 +856,12 @@ static int atmel_spi_set_xfer_speed(struct atmel_spi *as,
 {
 	u32			scbr, csr;
 	unsigned long		bus_hz;
+	u8 chip_select;
+
+	if (as->use_cs_gpios)
+		chip_select = 0;
+	else
+		chip_select = spi->chip_select;
 
 	/* v1 chips start out at half the peripheral bus speed. */
 	bus_hz = as->spi_clk;
@@ -872,9 +890,9 @@ static int atmel_spi_set_xfer_speed(struct atmel_spi *as,
 			xfer->speed_hz, scbr, bus_hz);
 		return -EINVAL;
 	}
-	csr = spi_readl(as, CSR0 + 4 * spi->chip_select);
+	csr = spi_readl(as, CSR0 + 4 * chip_select);
 	csr = SPI_BFINS(SCBR, scbr, csr);
-	spi_writel(as, CSR0 + 4 * spi->chip_select, csr);
+	spi_writel(as, CSR0 + 4 * chip_select, csr);
 
 	return 0;
 }
@@ -1238,8 +1256,12 @@ static int atmel_spi_setup(struct spi_device *spi)
 		"setup: bpw %u mode 0x%x -> csr%d %08x\n",
 		bits, spi->mode, spi->chip_select, csr);
 
-	if (!atmel_spi_is_v2(as))
-		spi_writel(as, CSR0 + 4 * spi->chip_select, csr);
+	if (!atmel_spi_is_v2(as)) {
+		if (as->use_cs_gpios)
+			spi_writel(as, CSR0, csr);
+		else
+			spi_writel(as, CSR0 + 4 * spi->chip_select, csr);
+	}
 
 	return 0;
 }
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH] spi: atmel: Remove AVR32 leftover
From: Gregory CLEMENT @ 2019-09-19 15:40 UTC (permalink / raw)
  To: Mark Brown, linux-spi, linux-kernel, Nicolas Ferre
  Cc: Thomas Petazzoni, Alexandre Belloni, Ludovic Desroches,
	linux-arm-kernel, Gregory CLEMENT

AV32 support has been from the kernel a few release ago, but there was
still some specific macro for this architecture in this driver. Lets
remove it.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/spi/spi-atmel.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index bb94f5927819..de1e1861a70c 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -222,37 +222,13 @@
 	  | SPI_BF(name, value))
 
 /* Register access macros */
-#ifdef CONFIG_AVR32
-#define spi_readl(port, reg) \
-	__raw_readl((port)->regs + SPI_##reg)
-#define spi_writel(port, reg, value) \
-	__raw_writel((value), (port)->regs + SPI_##reg)
-
-#define spi_readw(port, reg) \
-	__raw_readw((port)->regs + SPI_##reg)
-#define spi_writew(port, reg, value) \
-	__raw_writew((value), (port)->regs + SPI_##reg)
-
-#define spi_readb(port, reg) \
-	__raw_readb((port)->regs + SPI_##reg)
-#define spi_writeb(port, reg, value) \
-	__raw_writeb((value), (port)->regs + SPI_##reg)
-#else
 #define spi_readl(port, reg) \
 	readl_relaxed((port)->regs + SPI_##reg)
 #define spi_writel(port, reg, value) \
 	writel_relaxed((value), (port)->regs + SPI_##reg)
-
-#define spi_readw(port, reg) \
-	readw_relaxed((port)->regs + SPI_##reg)
 #define spi_writew(port, reg, value) \
 	writew_relaxed((value), (port)->regs + SPI_##reg)
 
-#define spi_readb(port, reg) \
-	readb_relaxed((port)->regs + SPI_##reg)
-#define spi_writeb(port, reg, value) \
-	writeb_relaxed((value), (port)->regs + SPI_##reg)
-#endif
 /* use PIO for small transfers, avoiding DMA setup/teardown overhead and
  * cache operations; better heuristics consider wordsize and bitrate.
  */
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* Re: [PATCH v4 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
From: Catalin Marinas @ 2019-09-19 15:41 UTC (permalink / raw)
  To: Kirill A. Shutemov
  Cc: Mark Rutland, linux-mm, Punit Agrawal, Will Deacon,
	Alex Van Brunt, Jia He, Marc Zyngier, Anshuman Khandual,
	Matthew Wilcox, Jun Yao, Kaly Xin, hejianet, Ralph Campbell,
	Suzuki Poulose, Jérôme Glisse, Thomas Gleixner,
	linux-arm-kernel, linux-kernel, James Morse, Andrew Morton,
	Robin Murphy, Kirill A. Shutemov
In-Reply-To: <20190919150007.k7scjplcya53j7r4@box>

On Thu, Sep 19, 2019 at 06:00:07PM +0300, Kirill A. Shutemov wrote:
> On Wed, Sep 18, 2019 at 07:00:30PM +0100, Catalin Marinas wrote:
> > On Wed, Sep 18, 2019 at 05:00:27PM +0300, Kirill A. Shutemov wrote:
> > > On Wed, Sep 18, 2019 at 09:19:14PM +0800, Jia He wrote:
> > > > @@ -2152,20 +2163,34 @@ static inline void cow_user_page(struct page *dst, struct page *src, unsigned lo
> > > >  	 */
> > > >  	if (unlikely(!src)) {
> > > >  		void *kaddr = kmap_atomic(dst);
> > > > -		void __user *uaddr = (void __user *)(va & PAGE_MASK);
> > > > +		void __user *uaddr = (void __user *)(addr & PAGE_MASK);
> > > > +		pte_t entry;
> > > >  
> > > >  		/*
> > > >  		 * This really shouldn't fail, because the page is there
> > > >  		 * in the page tables. But it might just be unreadable,
> > > >  		 * in which case we just give up and fill the result with
> > > > -		 * zeroes.
> > > > +		 * zeroes. On architectures with software "accessed" bits,
> > > > +		 * we would take a double page fault here, so mark it
> > > > +		 * accessed here.
> > > >  		 */
> > > > +		if (arch_faults_on_old_pte() && !pte_young(vmf->orig_pte)) {
> > > > +			spin_lock(vmf->ptl);
> > > > +			if (likely(pte_same(*vmf->pte, vmf->orig_pte))) {
> > > > +				entry = pte_mkyoung(vmf->orig_pte);
> > > > +				if (ptep_set_access_flags(vma, addr,
> > > > +							  vmf->pte, entry, 0))
> > > > +					update_mmu_cache(vma, addr, vmf->pte);
> > > > +			}
> > > 
> > > I don't follow.
> > > 
> > > So if pte has changed under you, you don't set the accessed bit, but never
> > > the less copy from the user.
> > > 
> > > What makes you think it will not trigger the same problem?
> > > 
> > > I think we need to make cow_user_page() fail in this case and caller --
> > > wp_page_copy() -- return zero. If the fault was solved by other thread, we
> > > are fine. If not userspace would re-fault on the same address and we will
> > > handle the fault from the second attempt.
> > 
> > It would be nice to clarify the semantics of this function and do as
> > you suggest but the current comment is slightly confusing:
> > 
> > 	/*
> > 	 * If the source page was a PFN mapping, we don't have
> > 	 * a "struct page" for it. We do a best-effort copy by
> > 	 * just copying from the original user address. If that
> > 	 * fails, we just zero-fill it. Live with it.
> > 	 */
> > 
> > Would any user-space rely on getting a zero-filled page here instead of
> > a recursive fault?
> 
> I don't see the point in zero-filled page in this case. SIGBUS sounds like
> more appropriate response, no?

I think misunderstood your comment. So, if !pte_same(), we should let
userspace re-fault. This wouldn't be a user ABI change and it is
bounded, can't end up in an infinite re-fault loop.

In case of a __copy_from_user_inatomic() error, SIGBUS would make more
sense but it changes the current behaviour (zero-filling the page). This
can be left for a separate patch, doesn't affect the arm64 case here.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* RE: [EXT] [PATCH v1] scsi: ufs: skip shutdown if hba is not powered
From: Bean Huo (beanhuo) @ 2019-09-19 15:49 UTC (permalink / raw)
  To: Stanley Chu, linux-scsi@vger.kernel.org,
	martin.petersen@oracle.com, avri.altman@wdc.com,
	alim.akhtar@samsung.com, pedrom.sousa@synopsys.com,
	jejb@linux.ibm.com
  Cc: andy.teng@mediatek.com, chun-hung.wu@mediatek.com,
	kuohong.wang@mediatek.com, linux-mediatek@lists.infradead.org,
	peter.wang@mediatek.com, matthias.bgg@gmail.com,
	linux-arm-kernel@lists.infradead.org
In-Reply-To: <1568780438-28753-1-git-send-email-stanley.chu@mediatek.com>

>To solve this issue, simply add checking to skip shutdown for above kind of
>situation.
>
>Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Acked-by: Bean Huo <beanhuo@micron.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v4 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
From: Kirill A. Shutemov @ 2019-09-19 15:51 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Mark Rutland, linux-mm, Punit Agrawal, Will Deacon,
	Alex Van Brunt, Jia He, Marc Zyngier, Anshuman Khandual,
	Matthew Wilcox, Jun Yao, Kaly Xin, hejianet, Ralph Campbell,
	Suzuki Poulose, Jérôme Glisse, Kirill A. Shutemov,
	Thomas Gleixner, linux-arm-kernel, linux-kernel, James Morse,
	Andrew Morton, Robin Murphy
In-Reply-To: <20190919154143.GA6472@arrakis.emea.arm.com>

On Thu, Sep 19, 2019 at 03:41:43PM +0000, Catalin Marinas wrote:
> On Thu, Sep 19, 2019 at 06:00:07PM +0300, Kirill A. Shutemov wrote:
> > On Wed, Sep 18, 2019 at 07:00:30PM +0100, Catalin Marinas wrote:
> > > On Wed, Sep 18, 2019 at 05:00:27PM +0300, Kirill A. Shutemov wrote:
> > > > On Wed, Sep 18, 2019 at 09:19:14PM +0800, Jia He wrote:
> > > > > @@ -2152,20 +2163,34 @@ static inline void cow_user_page(struct page *dst, struct page *src, unsigned lo
> > > > >  	 */
> > > > >  	if (unlikely(!src)) {
> > > > >  		void *kaddr = kmap_atomic(dst);
> > > > > -		void __user *uaddr = (void __user *)(va & PAGE_MASK);
> > > > > +		void __user *uaddr = (void __user *)(addr & PAGE_MASK);
> > > > > +		pte_t entry;
> > > > >  
> > > > >  		/*
> > > > >  		 * This really shouldn't fail, because the page is there
> > > > >  		 * in the page tables. But it might just be unreadable,
> > > > >  		 * in which case we just give up and fill the result with
> > > > > -		 * zeroes.
> > > > > +		 * zeroes. On architectures with software "accessed" bits,
> > > > > +		 * we would take a double page fault here, so mark it
> > > > > +		 * accessed here.
> > > > >  		 */
> > > > > +		if (arch_faults_on_old_pte() && !pte_young(vmf->orig_pte)) {
> > > > > +			spin_lock(vmf->ptl);
> > > > > +			if (likely(pte_same(*vmf->pte, vmf->orig_pte))) {
> > > > > +				entry = pte_mkyoung(vmf->orig_pte);
> > > > > +				if (ptep_set_access_flags(vma, addr,
> > > > > +							  vmf->pte, entry, 0))
> > > > > +					update_mmu_cache(vma, addr, vmf->pte);
> > > > > +			}
> > > > 
> > > > I don't follow.
> > > > 
> > > > So if pte has changed under you, you don't set the accessed bit, but never
> > > > the less copy from the user.
> > > > 
> > > > What makes you think it will not trigger the same problem?
> > > > 
> > > > I think we need to make cow_user_page() fail in this case and caller --
> > > > wp_page_copy() -- return zero. If the fault was solved by other thread, we
> > > > are fine. If not userspace would re-fault on the same address and we will
> > > > handle the fault from the second attempt.
> > > 
> > > It would be nice to clarify the semantics of this function and do as
> > > you suggest but the current comment is slightly confusing:
> > > 
> > > 	/*
> > > 	 * If the source page was a PFN mapping, we don't have
> > > 	 * a "struct page" for it. We do a best-effort copy by
> > > 	 * just copying from the original user address. If that
> > > 	 * fails, we just zero-fill it. Live with it.
> > > 	 */
> > > 
> > > Would any user-space rely on getting a zero-filled page here instead of
> > > a recursive fault?
> > 
> > I don't see the point in zero-filled page in this case. SIGBUS sounds like
> > more appropriate response, no?
> 
> I think misunderstood your comment. So, if !pte_same(), we should let
> userspace re-fault. This wouldn't be a user ABI change and it is
> bounded, can't end up in an infinite re-fault loop.

Right. !pte_same() can only happen if we raced with somebody else.
I cannot imagine situation when the race will happen more than few times
in a row.

> In case of a __copy_from_user_inatomic() error, SIGBUS would make more
> sense but it changes the current behaviour (zero-filling the page). This
> can be left for a separate patch, doesn't affect the arm64 case here.

I think it's safer to leave it as is. Maybe put WARN_ON_ONCE() or
something. There can be some obscure use-case that I don't see.

-- 
 Kirill A. Shutemov

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 17/23] mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()
From: Tudor.Ambarus @ 2019-09-19 15:54 UTC (permalink / raw)
  To: vigneshr, boris.brezillon, marek.vasut, miquel.raynal, richard,
	linux-mtd
  Cc: linux-aspeed, andrew, linux-kernel, vz, linux-mediatek, joel,
	matthias.bgg, computersforpeace, dwmw2, linux-arm-kernel
In-Reply-To: <dceca616-2b98-9bc8-73e4-32fb06fc753d@ti.com>



On 09/19/2019 05:33 PM, Vignesh Raghavendra wrote:
> Hi Tudor
> 

Hi, Vignesh,

> [...]
> 
> On 17-Sep-19 9:25 PM, Tudor.Ambarus@microchip.com wrote:
>> +static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 status_new,
>> +					    u8 mask)
>> +{
>> +	int ret;
>> +	u8 *sr_cr = nor->bouncebuf;
>> +	u8 cr_written;
>> +
>> +	/* Make sure we don't overwrite the contents of Status Register 2. */
>> +	if (!(nor->flags & SNOR_F_NO_READ_CR)) {
> Assuming SNOR_F_NO_READ_CR is not set...
> 
when SNOR_F_NO_READ_CR is not set, I read the Status Register 2 on the next line:

>> +		ret = spi_nor_read_cr(nor, &sr_cr[1]);
>> +		if (ret)
>> +			return ret;
>> +	} else if (nor->flash.quad_enable) {
>> +		/*
>> +		 * If the Status Register 2 Read command (35h) is not
>> +		 * supported, we should at least be sure we don't
>> +		 * change the value of the SR2 Quad Enable bit.
>> +		 *
>> +		 * We can safely assume that when the Quad Enable method is
>> +		 * set, the value of the QE bit is one, as a consequence of the
>> +		 * nor->flash.quad_enable() call.
>> +		 *
>> +		 * We can safely assume that the Quad Enable bit is present in
>> +		 * the Status Register 2 at BIT(1). According to the JESD216
>> +		 * revB standard, BFPT DWORDS[15], bits 22:20, the 16-bit
>> +		 * Write Status (01h) command is available just for the cases
>> +		 * in which the QE bit is described in SR2 at BIT(1).
>> +		 */

when SNOR_F_NO_READ_CR is set and nor->flash.quad_enable != NULL, Status
Register 2 (CR) is equal to CR_QUAD_EN_SPAN.

>> +		sr_cr[1] = CR_QUAD_EN_SPAN;
>> +	} else {

if SNOR_F_NO_READ_CR is set and nor->flash.quad_enable == NULL we don't need to
enable Quad Mode, so Status Register 2 is 0.

>> +		sr_cr[1] = 0;
>> +	}
>> +
> CR_QUAD_EN_SPAN will not be in sr_cr[1] when we reach here. So code
> won't enable quad mode.
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v4 3/5] locking/qspinlock: Introduce CNA into the slow path of qspinlock
From: Alex Kogan @ 2019-09-19 15:55 UTC (permalink / raw)
  To: Waiman Long
  Cc: linux-arch, guohanjun, arnd, peterz, dave.dice, jglauber, x86,
	will.deacon, linux, linux-kernel, rahul.x.yadav, mingo, bp, hpa,
	steven.sistare, tglx, daniel.m.jordan, linux-arm-kernel
In-Reply-To: <3ae2b6a2-ffe6-2ca1-e5bf-2292db50e26f@redhat.com>

>> +/*
>> + * cna_try_find_next - scan the main waiting queue looking for the first
>> + * thread running on the same NUMA node as the lock holder. If found (call it
>> + * thread T), move all threads in the main queue between the lock holder and
>> + * T to the end of the secondary queue and return T; otherwise, return NULL.
>> + *
>> + * Schematically, this may look like the following (nn stands for numa_node and
>> + * et stands for encoded_tail).
>> + *
>> + *     when cna_try_find_next() is called (the secondary queue is empty):
>> + *
>> + *  A+------------+   B+--------+   C+--------+   T+--------+
>> + *   |mcs:next    | -> |mcs:next| -> |mcs:next| -> |mcs:next| -> NULL
>> + *   |mcs:locked=1|    |cna:nn=0|    |cna:nn=2|    |cna:nn=1|
>> + *   |cna:nn=1    |    +--------+    +--------+    +--------+
>> + *   +----------- +
>> + *
>> + *     when cna_try_find_next() returns (the secondary queue contains B and C):
>> + *
>> + *  A+----------------+    T+--------+
>> + *   |mcs:next        | ->  |mcs:next| -> NULL
>> + *   |mcs:locked=B.et | -+  |cna:nn=1|
>> + *   |cna:nn=1        |  |  +--------+
>> + *   +--------------- +  |
>> + *                       |
>> + *                       +->  B+--------+   C+--------+
>> + *                             |mcs:next| -> |mcs:next|
>> + *                             |cna:nn=0|    |cna:nn=2|
>> + *                             |cna:tail| -> +--------+
>> + *                             +--------+
>> + *
>> + * The worst case complexity of the scan is O(n), where n is the number
>> + * of current waiters. However, the fast path, which is expected to be the
>> + * common case, is O(1).
>> + */
>> +static struct mcs_spinlock *cna_try_find_next(struct mcs_spinlock *node,
>> +					      struct mcs_spinlock *next)
>> +{
>> +	struct cna_node *cn = (struct cna_node *)node;
>> +	struct cna_node *cni = (struct cna_node *)next;
>> +	struct cna_node *first, *last = NULL;
>> +	int my_numa_node = cn->numa_node;
>> +
>> +	/* fast path: immediate successor is on the same NUMA node */
>> +	if (cni->numa_node == my_numa_node)
>> +		return next;
>> +
>> +	/* find any next waiter on 'our' NUMA node */
>> +	for (first = cni;
>> +	     cni && cni->numa_node != my_numa_node;
>> +	     last = cni, cni = (struct cna_node *)READ_ONCE(cni->mcs.next))
>> +		;
>> +
>> +	/* if found, splice any skipped waiters onto the secondary queue */
>> +	if (cni && last)
>> +		cna_splice_tail(cn, first, last);
>> +
>> +	return (struct mcs_spinlock *)cni;
>> +}
> 
> At the Linux Plumbers Conference last week, Will has raised the concern
> about the latency of the O(1) cna_try_find_next() operation that will
> add to the lock hold time.
While the worst case complexity of the scan is O(n), I _think it can be proven
that the amortized complexity is O(1). For intuition, consider a two-node 
system with N threads total. In the worst case scenario, the scan will go 
over N/2 threads running on a different node. If the scan ultimately “fails”
(no thread from the lock holder’s node is found), the lock will be passed
to the first thread from a different node and then between all those N/2 threads,
with a scan of just one node for the next N/2 - 1 passes. Otherwise, those 
N/2 threads will be moved to the secondary queue. On the next lock handover, 
we pass the lock either to the next thread in the main queue (as it has to be 
from our node) or to the first node in the secondary queue. In both cases, we 
scan just one node, and in the latter case, we have again N/2 - 1 passes with 
a scan of just one node each.

> One way to hide some of the latency is to do
> a pre-scan before acquiring the lock. The CNA code could override the
> pv_wait_head_or_lock() function to call cna_try_find_next() as a
> pre-scan and return 0. What do you think?
This is certainly possible, but I do not think it would completely eliminate 
the worst case scenario. It will probably make it even less likely, but at 
the same time, we will reduce the chance of actually finding a thread from the
same node (that may enter the main queue while we wait for the owner & pending 
to go away).

Regards,
— Alex
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] spi: atmel: Fix crash when using more than 4 gpio CS
From: Mark Brown @ 2019-09-19 16:03 UTC (permalink / raw)
  To: Gregory CLEMENT
  Cc: Alexandre Belloni, stable, linux-kernel, Ludovic Desroches,
	Thomas Petazzoni, linux-spi, linux-arm-kernel
In-Reply-To: <20190919153847.7179-1-gregory.clement@bootlin.com>


[-- Attachment #1.1: Type: text/plain, Size: 386 bytes --]

On Thu, Sep 19, 2019 at 05:38:47PM +0200, Gregory CLEMENT wrote:

> With this patch, when using a gpio CS, the hardware CS0 is always
> used. Thanks to this, there is no more limitation for the number of
> gpio CS we can use.

This is going to break any system where we use both a GPIO chip select
and chip select 0.  Ideally we'd try to figure out an unused chip select
to use here...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 00/23] mtd: spi-nor: Quad Enable and (un)lock methods
From: Tudor.Ambarus @ 2019-09-19 16:07 UTC (permalink / raw)
  To: vigneshr, boris.brezillon, marek.vasut, miquel.raynal, richard,
	linux-mtd
  Cc: linux-aspeed, andrew, linux-kernel, vz, linux-mediatek, joel,
	matthias.bgg, computersforpeace, dwmw2, linux-arm-kernel
In-Reply-To: <920a9946-af0d-1190-d59c-0b4acee71931@ti.com>



On 09/19/2019 05:37 PM, Vignesh Raghavendra wrote:
> External E-Mail
> 
> 
> Hi,

Hi, thanks for reviewing!

> 
> On 17-Sep-19 9:24 PM, Tudor.Ambarus@microchip.com wrote:
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
> [...]
>> Tudor Ambarus (23):
>>   mtd: spi-nor: hisi-sfc: Drop nor->erase NULL assignment
>>   mtd: spi-nor: Introduce 'struct spi_nor_controller_ops'
>>   mtd: spi-nor: cadence-quadspi: Fix cqspi_command_read() definition
>>   mtd: spi-nor: Rename nor->params to nor->flash
>>   mtd: spi-nor: Rework read_sr()
>>   mtd: spi-nor: Rework read_fsr()
>>   mtd: spi-nor: Rework read_cr()
>>   mtd: spi-nor: Rework write_enable/disable()
>>   mtd: spi-nor: Fix retlen handling in sst_write()
>>   mtd: spi-nor: Rework write_sr()
>>   mtd: spi-nor: Rework spi_nor_read/write_sr2()
>>   mtd: spi-nor: Report error in spi_nor_xread_sr()
>>   mtd: spi-nor: Void return type for spi_nor_clear_sr/fsr()
>>   mtd: spi-nor: Drop duplicated new line
>>   mtd: spi-nor: Drop spansion_quad_enable()
>>   mtd: spi-nor: Fix errno on quad_enable methods
>>   mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()
>>   mtd: spi-nor: Rework macronix_quad_enable()
>>   mtd: spi-nor: Rework spansion(_no)_read_cr_quad_enable()
>>   mtd: spi-nor: Update sr2_bit7_quad_enable()
>>   mtd: spi-nor: Rework the disabling of block write protection
>>   mtd: spi-nor: Add Global Block Unlock support
>>   mtd: spi-nor: Unlock global block protection on sst26vf064b
> 
> With whole series applied, I see:
> 
> drivers/mtd/spi-nor/spi-nor.c:520: warning: Function parameter or member 'cr' not described in 'spi_nor_read_cr'
> drivers/mtd/spi-nor/spi-nor.c:520: warning: Excess function parameter 'fsr' description in 'spi_nor_read_cr'
> drivers/mtd/spi-nor/spi-nor.c:742: warning: Function parameter or member 'len' not described in 'spi_nor_write_sr'
> drivers/mtd/spi-nor/spi-nor.c:889: warning: Function parameter or member 'status_new' not described in 'spi_nor_write_sr1_and_check'
> drivers/mtd/spi-nor/spi-nor.c:889: warning: Function parameter or member 'mask' not described in 'spi_nor_write_sr1_and_check'
> drivers/mtd/spi-nor/spi-nor.c:923: warning: Function parameter or member 'status_new' not described in 'spi_nor_write_16bit_sr_and_check'
> drivers/mtd/spi-nor/spi-nor.c:923: warning: Function parameter or member 'mask' not described in 'spi_nor_write_16bit_sr_and_check'
> drivers/mtd/spi-nor/spi-nor.c:997: warning: Function parameter or member 'status_new' not described in 'spi_nor_write_sr_and_check'
> drivers/mtd/spi-nor/spi-nor.c:997: warning: Function parameter or member 'mask' not described in 'spi_nor_write_sr_and_check'
> 
> Could you please fix up docs next time around?

I'll fix these, thanks!

I've just compiled the code and I can't see the warnings. What should I do to
get these warnings?

Thanks,
ta

> 
> Regards
> Vignesh
>>
>>  drivers/mtd/spi-nor/aspeed-smc.c      |   23 +-
>>  drivers/mtd/spi-nor/cadence-quadspi.c |   54 +-
>>  drivers/mtd/spi-nor/hisi-sfc.c        |   23 +-
>>  drivers/mtd/spi-nor/intel-spi.c       |   24 +-
>>  drivers/mtd/spi-nor/mtk-quadspi.c     |   25 +-
>>  drivers/mtd/spi-nor/nxp-spifi.c       |   23 +-
>>  drivers/mtd/spi-nor/spi-nor.c         | 1697 ++++++++++++++++++---------------
>>  include/linux/mtd/spi-nor.h           |   75 +-
>>  8 files changed, 1050 insertions(+), 894 deletions(-)
>>
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH v5 0/3] fix double page fault on arm64
From: Jia He @ 2019-09-19 16:12 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Mark Rutland, James Morse,
	Marc Zyngier, Matthew Wilcox, Kirill A. Shutemov,
	linux-arm-kernel, linux-kernel, linux-mm, Suzuki Poulose
  Cc: Ralph Campbell, Jia He, Anshuman Khandual, Alex Van Brunt,
	Kaly Xin, Jérôme Glisse, Punit Agrawal, hejianet,
	Andrew Morton, Robin Murphy, Thomas Gleixner

When we tested pmdk unit test vmmalloc_fork TEST1 in arm64 guest, there
will be a double page fault in __copy_from_user_inatomic of cow_user_page.

As told by Catalin: "On arm64 without hardware Access Flag, copying from
user will fail because the pte is old and cannot be marked young. So we
always end up with zeroed page after fork() + CoW for pfn mappings. we
don't always have a hardware-managed access flag on arm64."

Changes
v5: handle the case correctly when !pte_same
    fix kbuild test failed
v4: introduce cpu_has_hw_af (Suzuki)
    bail out if !pte_same (Kirill)
v3: add vmf->ptl lock/unlock (by Kirill A. Shutemov)
    add arch_faults_on_old_pte (Matthew, Catalin)
v2: remove FAULT_FLAG_WRITE when setting pte access flag (by Catalin)

Jia He (3):
  arm64: cpufeature: introduce helper cpu_has_hw_af()
  arm64: mm: implement arch_faults_on_old_pte() on arm64
  mm: fix double page fault on arm64 if PTE_AF is cleared

 arch/arm64/include/asm/cpufeature.h |  1 +
 arch/arm64/include/asm/pgtable.h    | 12 ++++++
 arch/arm64/kernel/cpufeature.c      | 10 +++++
 mm/memory.c                         | 59 ++++++++++++++++++++++++++---
 4 files changed, 77 insertions(+), 5 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH v5 1/3] arm64: cpufeature: introduce helper cpu_has_hw_af()
From: Jia He @ 2019-09-19 16:12 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Mark Rutland, James Morse,
	Marc Zyngier, Matthew Wilcox, Kirill A. Shutemov,
	linux-arm-kernel, linux-kernel, linux-mm, Suzuki Poulose
  Cc: Ralph Campbell, Jia He, Anshuman Khandual, Alex Van Brunt,
	Kaly Xin, Jérôme Glisse, Punit Agrawal, hejianet,
	Andrew Morton, Robin Murphy, Thomas Gleixner
In-Reply-To: <20190919161204.142796-1-justin.he@arm.com>

We unconditionally set the HW_AFDBM capability and only enable it on
CPUs which really have the feature. But sometimes we need to know
whether this cpu has the capability of HW AF. So decouple AF from
DBM by new helper cpu_has_hw_af().

Reported-by: kbuild test robot <lkp@intel.com>
Suggested-by: Suzuki Poulose <Suzuki.Poulose@arm.com>
Signed-off-by: Jia He <justin.he@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |  1 +
 arch/arm64/kernel/cpufeature.c      | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index c96ffa4722d3..206b6e3954cf 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -390,6 +390,7 @@ extern DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE);
 	for_each_set_bit(cap, cpu_hwcaps, ARM64_NCAPS)
 
 bool this_cpu_has_cap(unsigned int cap);
+bool cpu_has_hw_af(void);
 void cpu_set_feature(unsigned int num);
 bool cpu_have_feature(unsigned int num);
 unsigned long cpu_get_elf_hwcap(void);
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index b1fdc486aed8..fb0e9425d286 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1141,6 +1141,16 @@ static bool has_hw_dbm(const struct arm64_cpu_capabilities *cap,
 	return true;
 }
 
+/* Decouple AF from AFDBM. */
+bool cpu_has_hw_af(void)
+{
+	return (read_cpuid(ID_AA64MMFR1_EL1) & 0xf);
+}
+#else /* CONFIG_ARM64_HW_AFDBM */
+bool cpu_has_hw_af(void)
+{
+	return false;
+}
 #endif
 
 #ifdef CONFIG_ARM64_VHE
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v5 2/3] arm64: mm: implement arch_faults_on_old_pte() on arm64
From: Jia He @ 2019-09-19 16:12 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Mark Rutland, James Morse,
	Marc Zyngier, Matthew Wilcox, Kirill A. Shutemov,
	linux-arm-kernel, linux-kernel, linux-mm, Suzuki Poulose
  Cc: Ralph Campbell, Jia He, Anshuman Khandual, Alex Van Brunt,
	Kaly Xin, Jérôme Glisse, Punit Agrawal, hejianet,
	Andrew Morton, Robin Murphy, Thomas Gleixner
In-Reply-To: <20190919161204.142796-1-justin.he@arm.com>

On arm64 without hardware Access Flag, copying fromuser will fail because
the pte is old and cannot be marked young. So we always end up with zeroed
page after fork() + CoW for pfn mappings. we don't always have a
hardware-managed access flag on arm64.

Hence implement arch_faults_on_old_pte on arm64 to indicate that it might
cause page fault when accessing old pte.

Signed-off-by: Jia He <justin.he@arm.com>
---
 arch/arm64/include/asm/pgtable.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index e09760ece844..4a9939615e41 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -868,6 +868,18 @@ static inline void update_mmu_cache(struct vm_area_struct *vma,
 #define phys_to_ttbr(addr)	(addr)
 #endif
 
+/*
+ * On arm64 without hardware Access Flag, copying fromuser will fail because
+ * the pte is old and cannot be marked young. So we always end up with zeroed
+ * page after fork() + CoW for pfn mappings. we don't always have a
+ * hardware-managed access flag on arm64.
+ */
+static inline bool arch_faults_on_old_pte(void)
+{
+	return !cpu_has_hw_af();
+}
+#define arch_faults_on_old_pte arch_faults_on_old_pte
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ASM_PGTABLE_H */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v5 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
From: Jia He @ 2019-09-19 16:12 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, Mark Rutland, James Morse,
	Marc Zyngier, Matthew Wilcox, Kirill A. Shutemov,
	linux-arm-kernel, linux-kernel, linux-mm, Suzuki Poulose
  Cc: Ralph Campbell, Jia He, Anshuman Khandual, Alex Van Brunt,
	Kaly Xin, Jérôme Glisse, Punit Agrawal, hejianet,
	Andrew Morton, Robin Murphy, Thomas Gleixner
In-Reply-To: <20190919161204.142796-1-justin.he@arm.com>

When we tested pmdk unit test [1] vmmalloc_fork TEST1 in arm64 guest, there
will be a double page fault in __copy_from_user_inatomic of cow_user_page.

Below call trace is from arm64 do_page_fault for debugging purpose
[  110.016195] Call trace:
[  110.016826]  do_page_fault+0x5a4/0x690
[  110.017812]  do_mem_abort+0x50/0xb0
[  110.018726]  el1_da+0x20/0xc4
[  110.019492]  __arch_copy_from_user+0x180/0x280
[  110.020646]  do_wp_page+0xb0/0x860
[  110.021517]  __handle_mm_fault+0x994/0x1338
[  110.022606]  handle_mm_fault+0xe8/0x180
[  110.023584]  do_page_fault+0x240/0x690
[  110.024535]  do_mem_abort+0x50/0xb0
[  110.025423]  el0_da+0x20/0x24

The pte info before __copy_from_user_inatomic is (PTE_AF is cleared):
[ffff9b007000] pgd=000000023d4f8003, pud=000000023da9b003, pmd=000000023d4b3003, pte=360000298607bd3

As told by Catalin: "On arm64 without hardware Access Flag, copying from
user will fail because the pte is old and cannot be marked young. So we
always end up with zeroed page after fork() + CoW for pfn mappings. we
don't always have a hardware-managed access flag on arm64."

This patch fix it by calling pte_mkyoung. Also, the parameter is
changed because vmf should be passed to cow_user_page()

Add a WARN_ON_ONCE when __copy_from_user_inatomic() returns error
in case there can be some obscure use-case.(by Kirill)

[1] https://github.com/pmem/pmdk/tree/master/src/test/vmmalloc_fork

Reported-by: Yibo Cai <Yibo.Cai@arm.com>
Signed-off-by: Jia He <justin.he@arm.com>
---
 mm/memory.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 54 insertions(+), 5 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index e2bb51b6242e..cf681963b2f5 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -118,6 +118,13 @@ int randomize_va_space __read_mostly =
 					2;
 #endif
 
+#ifndef arch_faults_on_old_pte
+static inline bool arch_faults_on_old_pte(void)
+{
+	return false;
+}
+#endif
+
 static int __init disable_randmaps(char *s)
 {
 	randomize_va_space = 0;
@@ -2140,8 +2147,12 @@ static inline int pte_unmap_same(struct mm_struct *mm, pmd_t *pmd,
 	return same;
 }
 
-static inline void cow_user_page(struct page *dst, struct page *src, unsigned long va, struct vm_area_struct *vma)
+static inline int cow_user_page(struct page *dst, struct page *src,
+				struct vm_fault *vmf)
 {
+	struct vm_area_struct *vma = vmf->vma;
+	unsigned long addr = vmf->address;
+
 	debug_dma_assert_idle(src);
 
 	/*
@@ -2152,7 +2163,29 @@ static inline void cow_user_page(struct page *dst, struct page *src, unsigned lo
 	 */
 	if (unlikely(!src)) {
 		void *kaddr = kmap_atomic(dst);
-		void __user *uaddr = (void __user *)(va & PAGE_MASK);
+		void __user *uaddr = (void __user *)(addr & PAGE_MASK);
+		pte_t entry;
+
+		/* On architectures with software "accessed" bits, we would
+		 * take a double page fault, so mark it accessed here.
+		 */
+		if (arch_faults_on_old_pte() && !pte_young(vmf->orig_pte)) {
+			spin_lock(vmf->ptl);
+			if (likely(pte_same(*vmf->pte, vmf->orig_pte))) {
+				entry = pte_mkyoung(vmf->orig_pte);
+				if (ptep_set_access_flags(vma, addr,
+							  vmf->pte, entry, 0))
+					update_mmu_cache(vma, addr, vmf->pte);
+			} else {
+				/* Other thread has already handled the fault
+				 * and we don't need to do anything. If it's
+				 * not the case, the fault will be triggered
+				 * again on the same address.
+				 */
+				return -1;
+			}
+			spin_unlock(vmf->ptl);
+		}
 
 		/*
 		 * This really shouldn't fail, because the page is there
@@ -2160,12 +2193,17 @@ static inline void cow_user_page(struct page *dst, struct page *src, unsigned lo
 		 * in which case we just give up and fill the result with
 		 * zeroes.
 		 */
-		if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE))
+		if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE)) {
+			/* In case there can be some obscure use-case */
+			WARN_ON_ONCE(1);
 			clear_page(kaddr);
+		}
 		kunmap_atomic(kaddr);
 		flush_dcache_page(dst);
 	} else
-		copy_user_highpage(dst, src, va, vma);
+		copy_user_highpage(dst, src, addr, vma);
+
+	return 0;
 }
 
 static gfp_t __get_fault_gfp_mask(struct vm_area_struct *vma)
@@ -2318,7 +2356,16 @@ static vm_fault_t wp_page_copy(struct vm_fault *vmf)
 				vmf->address);
 		if (!new_page)
 			goto oom;
-		cow_user_page(new_page, old_page, vmf->address, vma);
+
+		if (cow_user_page(new_page, old_page, vmf)) {
+			/* COW failed, if the fault was solved by other,
+			 * it's fine. If not, userspace would re-fault on
+			 * the same address and we will handle the fault
+			 * from the second attempt.
+			 */
+			put_page(new_page);
+			goto normal;
+		}
 	}
 
 	if (mem_cgroup_try_charge_delay(new_page, mm, GFP_KERNEL, &memcg, false))
@@ -2420,6 +2467,8 @@ static vm_fault_t wp_page_copy(struct vm_fault *vmf)
 		}
 		put_page(old_page);
 	}
+
+normal:
 	return page_copied ? VM_FAULT_WRITE : 0;
 oom_free_new:
 	put_page(new_page);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox