* Re: [PATCH 0/6] Symbol namespaces
From: Arnd Bergmann @ 2018-07-23 14:28 UTC (permalink / raw)
To: Martijn Coenen
Cc: Linux Kernel Mailing List, Masahiro Yamada, Michal Marek,
Geert Uytterhoeven, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
the arch/x86 maintainers, Alan Stern, Greg Kroah-Hartman,
Oliver Neukum, Jessica Yu, Stephen Boyd, Philippe Ombredanne,
Kate Stewart, Sam Ravnborg, Linux Kbuild mailing list, linux-m68k
In-Reply-To: <20180716122125.175792-1-maco@android.com>
On Mon, Jul 16, 2018 at 2:21 PM, Martijn Coenen <maco@android.com> wrote:
> As of Linux 4.17, there are more than 30000 exported symbols
> in the kernel. There seems to be some consensus amongst
> kernel devs that the export surface is too large, and hard
> to reason about.
>
> Generally, these symbols fall in one of these categories:
> 1) Symbols actually meant for drivers
> 2) Symbols that are only exported because functionality is
> split over multiple modules, yet they really shouldn't
> be used by modules outside of their own subsystem
> 3) Symbols really only meant for in-tree use
>
> When module developers try to upstream their code, it
> regularly turns out that they are using exported symbols
> that they really shouldn't be using. This problem is even
> bigger for drivers that are currently out-of-tree, which
> may be using many symbols that they shouldn't be using,
> and that break when those symbols are removed or modified.
>
> This patch allows subsystem maintainers to partition their
> exported symbols into separate namespaces, and module
> authors to import such namespaces only when needed.
>
> This allows subsystem maintainers to more easily limit
> availability of these namespaced symbols to other parts of
> the kernel. It can also be used to partition the set of
> exported symbols for documentation purposes; for example,
> a set of symbols that is really only used for debugging
> could be in a "SUBSYSTEM_DEBUG" namespace.
This looks nice. I don't want to overload you with additional
requests, but it might be good to think about how this can
be taken further, if we want to actually convert large
parts of the kernel to it. I have two ideas:
- It would be nice to have a simple mechanism in Kconfig
to put all symbols in a module into a namespace that is
derived from KBUILD_MODNAME, to avoid having to
annotate every symbol separately. This could be similar
to how we ended up dealing with EXPORT_NO_SYMBOLS
in the linux-2.4 days, with the goal of eventually having
a namespace for each symbol. Similarly, we could pass
a number of default namespaces through the Makefile,
e.g. if we have a directory that has lots of drivers that all
import the symbols from a common subsystem module.
- If this is possible to implement, it would be great to have
a way to limit the globally visible symbols in a module
to the ones that are explicitly exported even when a that
module is built-in rather than loadable. Not sure how this
is best done though. A couple of things can be done with
objcopy, or possibly by reintroducing partial linking (which
was removed not too long ago).
Arnd
^ permalink raw reply
* Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
From: Yixun Lan @ 2018-07-23 14:28 UTC (permalink / raw)
To: Kevin Hilman
Cc: yixun.lan, Rob Herring, Jerome Brunet, Neil Armstrong,
Carlo Caione, Michael Turquette, Stephen Boyd, Miquèl Raynal,
Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai,
Jian Hu, linux-clk, linux-amlogic,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org
In-Reply-To: <7hbmay3ut6.fsf@baylibre.com>
HI Kevin
On 07/23/2018 10:12 PM, Kevin Hilman wrote:
> Yixun Lan <yixun.lan@amlogic.com> writes:
>
> [...]
>
>>>
>>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>>>
>>> IMO, this should be done as part of merging this series. Otherwise, we
>>> have duplicated code for the same thing.
>>
>> IMO, I'd leave this out of this series, since this patch series is quite
>> complete as itself. Although, the downside is code duplication.
>>
>> Still, I need to hear Jerome, or Kevin's option, to see if or how we
>> should proceed the eMMC's clock conversion.
>>
>> I could think of three option myself
>> 1) don't do the conversion, downside is code duplication, upside is NO
>> DT change, no compatibility issue
>> 2) add a syscon node into eMMC DT node, then only convert clock part
>> into this mmc-clkc model, while still leave other eMMC register access
>> as the usual iomap way (still no race condition)
>> 3) convert all eMMC register access by using regmap interface.
>>
>> both 2) and 3) need to update the DT.
>>
>> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
>> the worst way due to dramatically change (I think this was already
>> rejected in the previous discussion)
>
> Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
> step-by-step approach is fine (and preferred) by me.
>
> Phase 1:
> - add new mmc-clk provider
> - add NAND driver using new mmc-clk provider
> - boards using NAND should ensure emmc_c is disabled in DT
>
> This allows us to not touch the MMC driver or existing upstream
> bindings. Yes, this means there is duplicate code in the MMC driver and
> the new mmc-clk provider, but that can be removed in the next phase.
>
Great, the approach to address this issue is reasonable.
We'd like to focus on phase 1 first, thanks
> Phase 2:
> - convert MMC driver to use new mmc-clk provider
> - update MMC users in DT and bindings
>
Ok.
Yixun
^ permalink raw reply
* [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
From: Yixun Lan @ 2018-07-23 14:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <7hbmay3ut6.fsf@baylibre.com>
HI Kevin
On 07/23/2018 10:12 PM, Kevin Hilman wrote:
> Yixun Lan <yixun.lan@amlogic.com> writes:
>
> [...]
>
>>>
>>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>>>
>>> IMO, this should be done as part of merging this series. Otherwise, we
>>> have duplicated code for the same thing.
>>
>> IMO, I'd leave this out of this series, since this patch series is quite
>> complete as itself. Although, the downside is code duplication.
>>
>> Still, I need to hear Jerome, or Kevin's option, to see if or how we
>> should proceed the eMMC's clock conversion.
>>
>> I could think of three option myself
>> 1) don't do the conversion, downside is code duplication, upside is NO
>> DT change, no compatibility issue
>> 2) add a syscon node into eMMC DT node, then only convert clock part
>> into this mmc-clkc model, while still leave other eMMC register access
>> as the usual iomap way (still no race condition)
>> 3) convert all eMMC register access by using regmap interface.
>>
>> both 2) and 3) need to update the DT.
>>
>> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
>> the worst way due to dramatically change (I think this was already
>> rejected in the previous discussion)
>
> Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
> step-by-step approach is fine (and preferred) by me.
>
> Phase 1:
> - add new mmc-clk provider
> - add NAND driver using new mmc-clk provider
> - boards using NAND should ensure emmc_c is disabled in DT
>
> This allows us to not touch the MMC driver or existing upstream
> bindings. Yes, this means there is duplicate code in the MMC driver and
> the new mmc-clk provider, but that can be removed in the next phase.
>
Great, the approach to address this issue is reasonable.
We'd like to focus on phase 1 first, thanks
> Phase 2:
> - convert MMC driver to use new mmc-clk provider
> - update MMC users in DT and bindings
>
Ok.
Yixun
^ permalink raw reply
* [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
From: Yixun Lan @ 2018-07-23 14:28 UTC (permalink / raw)
To: linus-amlogic
In-Reply-To: <7hbmay3ut6.fsf@baylibre.com>
HI Kevin
On 07/23/2018 10:12 PM, Kevin Hilman wrote:
> Yixun Lan <yixun.lan@amlogic.com> writes:
>
> [...]
>
>>>
>>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>>>
>>> IMO, this should be done as part of merging this series. Otherwise, we
>>> have duplicated code for the same thing.
>>
>> IMO, I'd leave this out of this series, since this patch series is quite
>> complete as itself. Although, the downside is code duplication.
>>
>> Still, I need to hear Jerome, or Kevin's option, to see if or how we
>> should proceed the eMMC's clock conversion.
>>
>> I could think of three option myself
>> 1) don't do the conversion, downside is code duplication, upside is NO
>> DT change, no compatibility issue
>> 2) add a syscon node into eMMC DT node, then only convert clock part
>> into this mmc-clkc model, while still leave other eMMC register access
>> as the usual iomap way (still no race condition)
>> 3) convert all eMMC register access by using regmap interface.
>>
>> both 2) and 3) need to update the DT.
>>
>> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
>> the worst way due to dramatically change (I think this was already
>> rejected in the previous discussion)
>
> Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
> step-by-step approach is fine (and preferred) by me.
>
> Phase 1:
> - add new mmc-clk provider
> - add NAND driver using new mmc-clk provider
> - boards using NAND should ensure emmc_c is disabled in DT
>
> This allows us to not touch the MMC driver or existing upstream
> bindings. Yes, this means there is duplicate code in the MMC driver and
> the new mmc-clk provider, but that can be removed in the next phase.
>
Great, the approach to address this issue is reasonable.
We'd like to focus on phase 1 first, thanks
> Phase 2:
> - convert MMC driver to use new mmc-clk provider
> - update MMC users in DT and bindings
>
Ok.
Yixun
^ permalink raw reply
* Re: [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111
From: Sakari Ailus @ 2018-07-23 14:28 UTC (permalink / raw)
To: Jacopo Mondi; +Cc: mchehab, linux-media, linux-kernel, linux-renesas-soc
In-Reply-To: <1528730253-25135-3-git-send-email-jacopo+renesas@jmondi.org>
Hi Jacopo,
Apologies for the late review. Please see my comments below.
On Mon, Jun 11, 2018 at 05:17:33PM +0200, Jacopo Mondi wrote:
> Add V4L2 sensor driver for Aptina MT9V111 CMOS image sensor.
>
> The MT9V111 is a 1/4-Inch CMOS image sensor based on MT9V011 with an
> integrated Image Flow Processor.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> MAINTAINERS | 8 +
Could you move the MAINTAINERS change to the DT binding documentation
patch? Currently I presume adding a binding file which isn't accounted for
produces a checkpatch.pl warning.
> drivers/media/i2c/Kconfig | 12 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/mt9v111.c | 1297 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1318 insertions(+)
> create mode 100644 drivers/media/i2c/mt9v111.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a38e24a..2c2fe60 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9523,6 +9523,14 @@ F: Documentation/devicetree/bindings/media/i2c/mt9v032.txt
> F: drivers/media/i2c/mt9v032.c
> F: include/media/i2c/mt9v032.h
>
> +MT9V111 APTINA CAMERA SENSOR
> +M: Jacopo Mondi <jacopo@jmondi.org>
> +L: linux-media@vger.kernel.org
> +T: git git://linuxtv.org/media_tree.git
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt
> +F: drivers/media/i2c/mt9v111.c
> +
> MULTIFUNCTION DEVICES (MFD)
> M: Lee Jones <lee.jones@linaro.org>
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 341452f..0bd867d 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -841,6 +841,18 @@ config VIDEO_MT9V032
> This is a Video4Linux2 sensor-level driver for the Micron
> MT9V032 752x480 CMOS sensor.
>
> +config VIDEO_MT9V111
> + tristate "Aptina MT9V111 sensor support"
> + depends on I2C && VIDEO_V4L2
> + depends on MEDIA_CAMERA_SUPPORT
> + depends on OF
Why OF? I see no effective OF dependencies in the driver.
> + help
> + This is a Video4Linux2 sensor-level driver for the Aptina/Micron
> + MT9V111 sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called mt9v111.
> +
> config VIDEO_SR030PC30
> tristate "Siliconfile SR030PC30 sensor support"
> depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index d679d57..f0e8618 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_VIDEO_MT9T001) += mt9t001.o
> obj-$(CONFIG_VIDEO_MT9T112) += mt9t112.o
> obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o
> obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
> +obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o
> obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o
> obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o
> obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o
> diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c
> new file mode 100644
> index 0000000..36e7424
> --- /dev/null
> +++ b/drivers/media/i2c/mt9v111.c
> @@ -0,0 +1,1297 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * V4L2 sensor driver for Aptina MT9V111 image sensor
> + * Copyright (C) 2018 Jacopo Mondi <jacopo@jmondi.org>
> + *
> + * Based on mt9v032 driver
> + * Copyright (C) 2010, Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
> + *
> + * Base on mt9v011 driver
> + * Copyright (c) 2009 Mauro Carvalho Chehab <mchehab@kernel.org>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/videodev2.h>
> +#include <linux/v4l2-mediabus.h>
> +#include <linux/module.h>
> +
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-image-sizes.h>
> +#include <media/v4l2-subdev.h>
> +
> +/*
> + * MT9V111 is a 1/4-Inch CMOS digital image sensor with an integrated
> + * Image Flow Processing (IFP) engine and a sensor core loosely based on
> + * MT9V011.
> + *
> + * The IFP can produce several output image formats from the sensor core
> + * output, this driver currently supports only YUYV format permutations.
> + *
> + * As the auto exposure algorithms controls exposure time and modifies the
> + * frame rate, this driver disables auto exposure control, auto white balancing
> + * and auto flickering avoidance to allow manual frame rate control through
> + * s_frame_interval subdev operation or V4L2_CID_V/HBLANK controls.
> + *
> + * While it seems possible to instruct the auto-exposure control algorithm to
> + * respect a programmed frame rate when adjusting the pixel integration time,
> + * registers controlling this feature are not documented in the public
> + * available sensor manual used to develop this driver (09005aef80e90084,
> + * MT9V111_1.fm - Rev. G 1/05 EN).
> + */
> +
> +#define MT9V111_CHIP_ID_HIGH 0x82
> +#define MT9V111_CHIP_ID_LOW 0x3a
> +
> +#define MT9V111_R01_ADDR_SPACE 0x01
> +#define MT9V111_R01_IFP 0x01
> +#define MT9V111_R01_CORE 0x04
> +
> +#define MT9V111_IFP_R06_OPMODE_CTRL 0x06
> +#define MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN BIT(1)
> +#define MT9V111_IFP_R06_OPMODE_CTRL_AE_EN BIT(14)
> +#define MT9V111_IFP_R07_IFP_RESET 0x07
> +#define MT9V111_IFP_R07_IFP_RESET_MASK BIT(0)
> +#define MT9V111_IFP_R08_OUTFMT_CTRL 0x08
> +#define MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER BIT(11)
> +#define MT9V111_IFP_R08_OUTFMT_CTRL_PCLK BIT(5)
> +#define MT9V111_IFP_R3A_OUTFMT_CTRL2 0x3a
> +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR BIT(0)
> +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC BIT(1)
> +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK GENMASK(2, 0)
> +#define MT9V111_IFP_RA5_HPAN 0xa5
> +#define MT9V111_IFP_RA6_HZOOM 0xa6
> +#define MT9V111_IFP_RA7_HOUT 0xa7
> +#define MT9V111_IFP_RA8_VPAN 0xa8
> +#define MT9V111_IFP_RA9_VZOOM 0xa9
> +#define MT9V111_IFP_RAA_VOUT 0xaa
> +#define MT9V111_IFP_DECIMATION_MASK GENMASK(9, 0)
> +#define MT9V111_IFP_DECIMATION_FREEZE BIT(15)
> +
> +#define MT9V111_CORE_R03_WIN_HEIGHT 0x03
> +#define MT9V111_CORE_R03_WIN_V_OFFS 2
> +#define MT9V111_CORE_R04_WIN_WIDTH 0x04
> +#define MT9V111_CORE_R04_WIN_H_OFFS 114
> +#define MT9V111_CORE_R05_HBLANK 0x05
> +#define MT9V111_CORE_R05_MIN_HBLANK 0x09
> +#define MT9V111_CORE_R05_MAX_HBLANK GENMASK(9, 0)
> +#define MT9V111_CORE_R05_DEF_HBLANK 0x26
> +#define MT9V111_CORE_R06_VBLANK 0x06
> +#define MT9V111_CORE_R06_MIN_VBLANK 0x03
> +#define MT9V111_CORE_R06_MAX_VBLANK GENMASK(11, 0)
> +#define MT9V111_CORE_R06_DEF_VBLANK 0x04
> +#define MT9V111_CORE_R07_OUT_CTRL 0x07
> +#define MT9V111_CORE_R07_OUT_CTRL_SAMPLE BIT(4)
> +#define MT9V111_CORE_R09_PIXEL_INT 0x09
> +#define MT9V111_CORE_R09_PIXEL_INT_MASK GENMASK(11, 0)
> +#define MT9V111_CORE_R0D_CORE_RESET 0x0d
> +#define MT9V111_CORE_R0D_CORE_RESET_MASK BIT(0)
> +#define MT9V111_CORE_RFF_CHIP_VER 0xff
> +
> +#define MT9V111_PIXEL_ARRAY_WIDTH 640
> +#define MT9V111_PIXEL_ARRAY_HEIGHT 480
> +
> +#define MT9V111_MAX_CLKIN 27000000
> +
> +struct mt9v111_dev {
> + struct device *dev;
> + struct i2c_client *client;
> +
> + /* Protects address space change operations. */
> + spinlock_t addr_lock;
> + u8 addr_space;
> +
> + struct v4l2_subdev sd;
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + struct media_pad pad;
> +#endif
> +
> + struct v4l2_ctrl *hblank;
> + struct v4l2_ctrl *vblank;
> + struct v4l2_ctrl_handler ctrls;
> +
> + /* Output image format and sizes. */
> + struct v4l2_mbus_framefmt fmt;
> + unsigned int fps;
> +
> + /* Protects power up/down sequences. */
> + struct mutex pwr_mutex;
> + int pwr_count;
> +
> + /* Protects stream on/off sequences. */
> + struct mutex stream_mutex;
> + bool streaming;
> +
> + /* Flags to mark HW settings as not yet applied. */
> + bool pending;
> +
> + /* Clock provider and system clock frequency. */
> + struct clk *clk;
> + u32 sysclk;
> +
> + struct gpio_desc *oe;
> + struct gpio_desc *standby;
> + struct gpio_desc *reset;
> +};
> +
> +#define sd_to_mt9v111(__sd) container_of(__sd, struct mt9v111_dev, sd)
> +
> +/*
> + * mt9v111_mbus_fmt - List all media bus formats supported by the driver.
> + *
> + * Only list the media bus code here. The image sizes are freely configurable
> + * in the pixel array sizes range.
> + *
> + * The frame desired frame interval, in the supported frame interval range, is
> + * obtained by configuring blanking as the sensor does not have a PLL but
> + * only a fixed clock divider that generates the output pixel clock.
> + */
> +static struct mt9v111_mbus_fmt {
> + u32 code;
> +} mt9v111_formats[] = {
> + {
> + .code = MEDIA_BUS_FMT_UYVY8_2X8,
> + },
> + {
> + .code = MEDIA_BUS_FMT_YUYV8_2X8,
> + },
> + {
> + .code = MEDIA_BUS_FMT_VYUY8_2X8,
> + },
> + {
> + .code = MEDIA_BUS_FMT_YVYU8_2X8,
> + },
> +};
> +
> +static u32 mt9v111_frame_intervals[] = {5, 10, 15, 20, 30};
> +
> +/*
> + * mt9v111_frame_sizes - List sensor's supported resolutions.
> + *
> + * Resolution generated through decimation in the IFP block from the
> + * full VGA pixel array.
> + */
> +static struct v4l2_rect mt9v111_frame_sizes[] = {
> + {
> + .width = 640,
> + .height = 480,
> + },
> + {
> + .width = 352,
> + .height = 288
> + },
> + {
> + .width = 320,
> + .height = 240,
> + },
> + {
> + .width = 176,
> + .height = 144,
> + },
> + {
> + .width = 160,
> + .height = 120,
> + },
> +};
> +
> +/* Initial register configuration. */
> +static struct mt9v111_reg_list {
> + u8 space;
> + u8 reg;
> + u16 mask;
> + u16 val;
> +} mt9v111_init_config[] = {
> + {
> + /* Disable AE and AWB to enable manual frame rate control. */
> + .space = MT9V111_R01_IFP,
> + .reg = MT9V111_IFP_R06_OPMODE_CTRL,
> + .mask = MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN |
> + MT9V111_IFP_R06_OPMODE_CTRL_AE_EN,
> + .val = 0,
> + },
> + {
> + /* Disable anti-flicker to enable manual frame rate control. */
> + .space = MT9V111_R01_IFP,
> + .reg = MT9V111_IFP_R08_OUTFMT_CTRL,
> + .mask = MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER,
> + .val = 0,
> + },
> +
> + /* TODO: AE and AWB manual configuration. */
> +};
> +
> +/* --- Device I/O access --- */
> +
> +static int __mt9v111_read(struct i2c_client *c, u8 reg, u16 *val)
> +{
> + struct i2c_msg msg[2];
> + __be16 buf;
> + int ret;
> +
> + msg[0].addr = c->addr;
> + msg[0].flags = 0;
> + msg[0].len = 1;
> + msg[0].buf = ®
> +
> + msg[1].addr = c->addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = 2;
> + msg[1].buf = (char *)&buf;
> +
> + ret = i2c_transfer(c->adapter, msg, 2);
> + if (ret < 0) {
> + dev_err(&c->dev, "i2c read transfer error: %d\n", ret);
> + return ret;
> + }
> +
> + *val = be16_to_cpu(buf);
> +
> + dev_info(&c->dev, "%s: %x=%x\n", __func__, reg, *val);
> +
> + return 0;
> +}
> +
> +static int __mt9v111_write(struct i2c_client *c, u8 reg, u16 val)
> +{
> + struct i2c_msg msg;
> + u8 buf[3] = { 0 };
> + int ret;
> +
> + buf[0] = reg;
> + buf[1] = val >> 8;
> + buf[2] = val & 0xff;
> +
> + msg.addr = c->addr;
> + msg.flags = 0;
> + msg.len = 3;
> + msg.buf = (char *)buf;
> +
> + dev_info(&c->dev, "%s: %x = %x%x\n", __func__, reg, buf[1], buf[2]);
> +
> + ret = i2c_transfer(c->adapter, &msg, 1);
> + if (ret < 0) {
> + dev_err(&c->dev, "i2c write transfer error: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int __mt9v111_addr_space_select(struct i2c_client *c, u16 addr_space)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(c);
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd);
> + u16 val;
> + int ret;
> +
> + spin_lock(&mt9v111->addr_lock);
> + if (mt9v111->addr_space == addr_space) {
> + spin_unlock(&mt9v111->addr_lock);
> + return 0;
> + }
> + spin_unlock(&mt9v111->addr_lock);
> +
> + ret = __mt9v111_write(c, MT9V111_R01_ADDR_SPACE, addr_space);
> + if (ret)
> + return ret;
> +
> + /* Verify address space has been updated */
> + ret = __mt9v111_read(c, MT9V111_R01_ADDR_SPACE, &val);
> + if (ret)
> + return ret;
> +
> + if (val != addr_space)
> + return -EINVAL;
> +
> + mt9v111->addr_space = addr_space;
> +
> + return 0;
> +}
> +
> +static int mt9v111_read(struct i2c_client *c, u8 addr_space, u8 reg, u16 *val)
> +{
> + int ret;
> +
> + /* Select register address space first. */
> + ret = __mt9v111_addr_space_select(c, addr_space);
> + if (ret)
> + return ret;
> +
> + ret = __mt9v111_read(c, reg, val);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int mt9v111_write(struct i2c_client *c, u8 addr_space, u8 reg, u16 val)
> +{
> + int ret;
> +
> + /* Select register address space first. */
> + ret = __mt9v111_addr_space_select(c, addr_space);
> + if (ret)
> + return ret;
> +
> + ret = __mt9v111_write(c, reg, val);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int mt9v111_update(struct i2c_client *c, u8 addr_space, u8 reg,
> + u16 mask, u16 val)
> +{
> + u16 current_val;
> + int ret;
> +
> + /* Select register address space first. */
> + ret = __mt9v111_addr_space_select(c, addr_space);
> + if (ret)
> + return ret;
> +
> + /* Read the current register value, then update it. */
> + ret = __mt9v111_read(c, reg, ¤t_val);
> + if (ret)
> + return ret;
> +
> + current_val &= ~mask;
> + current_val |= (val & mask);
> + ret = __mt9v111_write(c, reg, current_val);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int mt9v111_update_reglist(struct i2c_client *c,
> + struct mt9v111_reg_list *reg_list,
> + unsigned int n_regs)
> +{
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < n_regs; i++) {
> + struct mt9v111_reg_list *reg = ®_list[i];
> +
> + ret = mt9v111_update(c, reg->space, reg->reg, reg->mask,
> + reg->val);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/* --- Sensor HW operations --- */
> +
> +static int __mt9v111_power_on(struct v4l2_subdev *sd)
> +{
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd);
> + int ret;
> +
> + ret = clk_prepare_enable(mt9v111->clk);
> + if (ret)
> + return ret;
> +
> + clk_set_rate(mt9v111->clk, mt9v111->sysclk);
> +
> + gpiod_set_value(mt9v111->standby, 0);
> + usleep_range(500, 1000);
> +
> + gpiod_set_value(mt9v111->oe, 1);
> + usleep_range(500, 1000);
> +
> + return 0;
> +}
> +
> +static int __mt9v111_power_off(struct v4l2_subdev *sd)
> +{
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd);
> +
> + gpiod_set_value(mt9v111->oe, 0);
> + usleep_range(500, 1000);
> +
> + gpiod_set_value(mt9v111->standby, 1);
> + usleep_range(500, 1000);
> +
> + clk_disable_unprepare(mt9v111->clk);
> +
> + return 0;
> +}
> +
> +static int __mt9v111_hw_reset(struct mt9v111_dev *mt9v111)
> +{
> + if (!mt9v111->reset)
> + return -EINVAL;
> +
> + gpiod_set_value(mt9v111->reset, 1);
> + usleep_range(500, 1000);
> +
> + gpiod_set_value(mt9v111->reset, 0);
> + usleep_range(500, 1000);
> +
> + return 0;
> +}
> +
> +static int __mt9v111_sw_reset(struct mt9v111_dev *mt9v111)
> +{
> + struct i2c_client *c = mt9v111->client;
> + unsigned int ret;
> +
> + /* Software reset core and IFP blocks. */
> +
> + ret = mt9v111_update(c, MT9V111_R01_CORE,
> + MT9V111_CORE_R0D_CORE_RESET,
> + MT9V111_CORE_R0D_CORE_RESET_MASK, 1);
> + if (ret)
> + return ret;
> + usleep_range(500, 1000);
> +
> + ret = mt9v111_update(c, MT9V111_R01_CORE,
> + MT9V111_CORE_R0D_CORE_RESET,
> + MT9V111_CORE_R0D_CORE_RESET_MASK, 0);
> + if (ret)
> + return ret;
> + usleep_range(500, 1000);
> +
> + ret = mt9v111_update(c, MT9V111_R01_IFP,
> + MT9V111_IFP_R07_IFP_RESET,
> + MT9V111_IFP_R07_IFP_RESET_MASK, 1);
> + if (ret)
> + return ret;
> + usleep_range(500, 1000);
> +
> + ret = mt9v111_update(c, MT9V111_R01_IFP,
> + MT9V111_IFP_R07_IFP_RESET,
> + MT9V111_IFP_R07_IFP_RESET_MASK, 0);
> + if (ret)
> + return ret;
> + usleep_range(500, 1000);
> +
> + return 0;
> +}
> +
> +static int mt9v111_calc_frame_rate(struct mt9v111_dev *mt9v111,
> + struct v4l2_fract *tpf)
> +{
> + unsigned int fps = tpf->numerator ?
> + tpf->denominator / tpf->numerator :
> + tpf->denominator;
> + unsigned int best_diff;
> + unsigned int frm_cols;
> + unsigned int row_pclk;
> + unsigned int best_fps;
> + unsigned int pclk;
> + unsigned int diff;
> + unsigned int idx;
> + unsigned int hb;
> + unsigned int vb;
> + unsigned int i;
> + int ret;
> +
> + /* Approximate to the closest supported frame interval. */
> + best_diff = ~0L;
> + for (i = 0, idx = 0; i < ARRAY_SIZE(mt9v111_frame_intervals); i++) {
> + diff = abs(fps - mt9v111_frame_intervals[i]);
> + if (diff < best_diff) {
> + idx = i;
> + best_diff = diff;
> + }
> + }
> + fps = mt9v111_frame_intervals[idx];
> +
> + /*
> + * The sensor does not provide a PLL circuitry and pixel clock is
> + * generated dividing by two the master clock source.
> + *
> + * Trow = (W + Hblank + 114) * 2 * (1 / SYSCLK)
> + * TFrame = Trow * (H + Vblank + 2)
> + *
> + * FPS = (SYSCLK / 2) / (Trow * (H + Vblank + 2))
> + *
> + * This boils down to tune H and V blanks to best approximate the
> + * above equation.
> + *
> + * Test all available H/V blank values, until we reach the
> + * desired frame rate.
> + */
> + best_fps = vb = hb = 0;
> + pclk = DIV_ROUND_CLOSEST(mt9v111->sysclk, 2);
> + row_pclk = MT9V111_PIXEL_ARRAY_WIDTH + 7 + MT9V111_CORE_R04_WIN_H_OFFS;
> + frm_cols = MT9V111_PIXEL_ARRAY_HEIGHT + 7 + MT9V111_CORE_R03_WIN_V_OFFS;
> +
> + best_diff = ~0L;
> + for (vb = MT9V111_CORE_R06_MIN_VBLANK;
> + vb < MT9V111_CORE_R06_MAX_VBLANK; vb++) {
> + for (hb = MT9V111_CORE_R05_MIN_HBLANK;
> + hb < MT9V111_CORE_R05_MAX_HBLANK; hb += 10) {
> + unsigned int t_frame = (row_pclk + hb) *
> + (frm_cols + vb);
> + unsigned int t_fps = DIV_ROUND_CLOSEST(pclk, t_frame);
> +
> + diff = abs(fps - t_fps);
> + if (diff < best_diff) {
> + best_diff = diff;
> + best_fps = t_fps;
> +
> + if (diff == 0)
> + break;
> + }
> + }
> +
> + if (diff == 0)
> + break;
> + }
> +
> + dev_info(mt9v111->dev, "FPS: %u, hb = %u, vb = %u\n", fps, hb, vb);
> +
> + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->hblank, hb);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->vblank, vb);
> + if (ret)
> + return ret;
> +
> + tpf->numerator = 1;
> + tpf->denominator = best_fps;
> +
> + return 0;
> +}
> +
> +static int mt9v111_hw_config(struct mt9v111_dev *mt9v111)
> +{
> + struct i2c_client *c = mt9v111->client;
> + unsigned int ret;
> + u16 outfmtctrl2;
> +
> + /* Force device reset. */
> + ret = __mt9v111_hw_reset(mt9v111);
> + if (ret == -EINVAL)
> + ret = __mt9v111_sw_reset(mt9v111);
> + if (ret)
> + return ret;
> +
> + /*
> + * Disable AE, AWB and anti-flickering to allow manual frame rate
> + * control.
> + */
> + ret = mt9v111_update_reglist(c, mt9v111_init_config,
> + ARRAY_SIZE(mt9v111_init_config));
> + if (ret)
> + return ret;
> +
> + /* Configure internal clock sample rate. */
> + ret = mt9v111->sysclk < DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ?
> + mt9v111_update(c, MT9V111_R01_CORE,
> + MT9V111_CORE_R07_OUT_CTRL,
> + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 1) :
> + mt9v111_update(c, MT9V111_R01_CORE,
> + MT9V111_CORE_R07_OUT_CTRL,
> + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 0);
> + if (ret)
> + return ret;
> +
> + /*
> + * Configure output image format components ordering.
> + *
> + * TODO: IFP block can also output several RGB permutations, we only
> + * support YUYV permutations at the moment.
> + */
> + switch (mt9v111->fmt.code) {
> + case MEDIA_BUS_FMT_YUYV8_2X8:
> + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC;
> + break;
> + case MEDIA_BUS_FMT_VYUY8_2X8:
> + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR;
> + break;
> + case MEDIA_BUS_FMT_YVYU8_2X8:
> + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC |
> + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR;
> + break;
> + case MEDIA_BUS_FMT_UYVY8_2X8:
> + default:
> + outfmtctrl2 = 0;
> + break;
> + }
> +
> + ret = mt9v111_update(c, MT9V111_R01_IFP, MT9V111_IFP_R3A_OUTFMT_CTRL2,
> + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK,
> + outfmtctrl2);
> + if (ret)
> + return ret;
> +
> + /*
> + * Do not change default sensor's core configuration:
> + * output the whole 640x480 pixel array, skip 18 columns and 6 rows.
> + *
> + * Instead, control the output image size through IFP block.
> + *
> + * TODO: No zoom&pan support. Currently we control the output image
> + * size only through decimation, with no zoom support.
> + */
> + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA5_HPAN,
> + MT9V111_IFP_DECIMATION_FREEZE);
> + if (ret)
> + return ret;
> +
> + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA8_VPAN,
> + MT9V111_IFP_DECIMATION_FREEZE);
> + if (ret)
> + return ret;
> +
> + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA6_HZOOM,
> + MT9V111_IFP_DECIMATION_FREEZE |
> + MT9V111_PIXEL_ARRAY_WIDTH);
> + if (ret)
> + return ret;
> +
> + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA9_VZOOM,
> + MT9V111_IFP_DECIMATION_FREEZE |
> + MT9V111_PIXEL_ARRAY_HEIGHT);
> + if (ret)
> + return ret;
> +
> + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA7_HOUT,
> + MT9V111_IFP_DECIMATION_FREEZE |
> + mt9v111->fmt.width);
> + if (ret)
> + return ret;
> +
> + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RAA_VOUT,
> + mt9v111->fmt.height);
> + if (ret)
> + return ret;
> +
> + /* Update blankings to match the programmed frame rate. */
> + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R05_HBLANK,
> + MT9V111_CORE_R05_MAX_HBLANK,
> + mt9v111->hblank->val);
> + if (ret)
> + return ret;
> +
> + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R06_VBLANK,
> + MT9V111_CORE_R06_MAX_VBLANK,
> + mt9v111->vblank->val);
> + if (ret)
> + return ret;
> +
> + /*
> + * Set pixel integration time to the whole frame time.
> + * This value controls the the shutter delay when running with AE
> + * disabled. If longer than frame time, it affects the output
> + * frame rate.
> + */
> + ret = mt9v111_write(c, MT9V111_R01_CORE, MT9V111_CORE_R09_PIXEL_INT,
> + MT9V111_PIXEL_ARRAY_HEIGHT);
return mt9v111_write(...);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +/* --- V4L2 subdev operations --- */
> +
> +static int mt9v111_s_power(struct v4l2_subdev *sd, int on)
Using runtime PM would be nicer. See e.g. the smiapp driver for an example.
This could be done as a separate change later on as well.
> +{
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd);
> + int pwr_count;
> + int ret = 0;
> +
> + mutex_lock(&mt9v111->pwr_mutex);
> +
> + /*
> + * Make sure we're transitioning from 0 to 1, or viceversa, only
> + * before actually changing the power state.
> + */
> + pwr_count = mt9v111->pwr_count;
> + pwr_count += on ? 1 : -1;
> + if (pwr_count == !!on) {
> + ret = on ? __mt9v111_power_on(sd) :
> + __mt9v111_power_off(sd);
> + if (!ret)
> + /* All went well, updated power counter. */
> + mt9v111->pwr_count = pwr_count;
> +
> + mutex_unlock(&mt9v111->pwr_mutex);
> +
> + return ret;
> + }
> +
> + /*
> + * Update power counter to keep track of how many nested calls we
> + * received.
> + */
> + WARN_ON(pwr_count < 0 || pwr_count > 1);
> + mt9v111->pwr_count = pwr_count;
> +
> + mutex_unlock(&mt9v111->pwr_mutex);
> +
> + return ret;
> +}
> +
> +static int mt9v111_s_stream(struct v4l2_subdev *subdev, int enable)
> +{
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev);
> + int ret;
> +
> + mutex_lock(&mt9v111->stream_mutex);
> +
> + if (mt9v111->streaming == enable) {
> + mutex_unlock(&mt9v111->stream_mutex);
> + return 0;
> + }
> +
> + ret = mt9v111_s_power(subdev, enable);
> + if (ret)
> + goto error_unlock;
> +
> + if (enable && mt9v111->pending) {
> + ret = mt9v111_hw_config(mt9v111);
> + if (ret)
> + goto error_unlock;
> +
> + /*
> + * No need to update control here as far as only H/VBLANK are
> + * supported and immediately programmed to registers in .s_ctrl
> + */
> +
> + mt9v111->pending = false;
> + }
> +
> + mt9v111->streaming = enable ? true : false;
> + mutex_unlock(&mt9v111->stream_mutex);
> +
> + return 0;
> +
> +error_unlock:
> + mutex_unlock(&mt9v111->stream_mutex);
> +
> + return ret;
> +}
> +
> +static int mt9v111_s_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_frame_interval *ival)
> +{
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd);
> + struct v4l2_fract *tpf = &ival->interval;
> + unsigned int fps = tpf->numerator ?
> + tpf->denominator / tpf->numerator :
> + tpf->denominator;
> + unsigned int max_fps;
> +
> + mutex_lock(&mt9v111->stream_mutex);
> + if (mt9v111->streaming) {
> + mutex_unlock(&mt9v111->stream_mutex);
> + return -EBUSY;
> + }
> + mutex_unlock(&mt9v111->stream_mutex);
> +
> + if (mt9v111->fps == fps)
> + return 0;
> +
> + /* Make sure frame rate/image sizes constraints are respected. */
> + if (mt9v111->fmt.width < QVGA_WIDTH &&
> + mt9v111->fmt.height < QVGA_HEIGHT)
> + max_fps = 90;
> + else if (mt9v111->fmt.width < CIF_WIDTH &&
> + mt9v111->fmt.height < CIF_HEIGHT)
> + max_fps = 60;
> + else
> + max_fps = mt9v111->sysclk <
> + DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? 15 :
> + 30;
> +
> + if (fps > max_fps)
> + return -EINVAL;
> +
> + mt9v111_calc_frame_rate(mt9v111, tpf);
> +
> + mt9v111->fps = fps;
> + mt9v111->pending = true;
I think you'd need to keep the mutex until here.
> +
> + return 0;
> +}
> +
> +static int mt9v111_g_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_frame_interval *ival)
> +{
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd);
> + struct v4l2_fract *tpf = &ival->interval;
> +
> + tpf->numerator = 1;
> + tpf->denominator = mt9v111->fps;
Same here --- access to fps is serialised by the mutex.
> +
> + return 0;
> +}
> +
> +static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format(
> + struct mt9v111_dev *mt9v111,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad,
> + enum v4l2_subdev_format_whence which)
> +{
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> +#ifdef CONFIG_MEDIA_CONTROLLER
> + return v4l2_subdev_get_try_format(&mt9v111->sd, cfg, pad);
> +#else
> + return &cfg->try_fmt;
> +#endif
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return &mt9v111->fmt;
> + default:
> + return NULL;
> + }
> +}
> +
> +static int mt9v111_enum_mbus_code(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + if (code->pad || code->index > ARRAY_SIZE(mt9v111_formats) - 1)
> + return -EINVAL;
> +
> + code->code = mt9v111_formats[code->index].code;
> +
> + return 0;
> +}
> +
> +static int mt9v111_enum_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_interval_enum *fie)
> +{
> + unsigned int i;
> +
> + if (fie->pad || fie->index >= ARRAY_SIZE(mt9v111_frame_intervals))
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++)
> + if (fie->width == mt9v111_frame_sizes[i].width &&
> + fie->height == mt9v111_frame_sizes[i].height)
> + break;
> +
> + if (i == ARRAY_SIZE(mt9v111_frame_sizes))
> + return -EINVAL;
> +
> + fie->interval.numerator = 1;
> + fie->interval.denominator = mt9v111_frame_intervals[fie->index];
> +
> + return 0;
> +}
> +
> +static int mt9v111_enum_frame_size(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + if (fse->pad || fse->index > ARRAY_SIZE(mt9v111_frame_sizes))
> + return -EINVAL;
> +
> + fse->min_width = mt9v111_frame_sizes[fse->index].width;
> + fse->max_width = mt9v111_frame_sizes[fse->index].width;
> + fse->min_height = mt9v111_frame_sizes[fse->index].height;
> + fse->max_height = mt9v111_frame_sizes[fse->index].height;
> +
> + return 0;
> +}
> +
> +static int mt9v111_get_format(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *format)
> +{
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev);
> +
> + if (format->pad)
> + return -EINVAL;
> +
> + format->format = *__mt9v111_get_pad_format(mt9v111, cfg, format->pad,
> + format->which);
Ditto.
> + return 0;
> +}
> +
> +static int mt9v111_set_format(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *format)
> +{
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev);
> + struct v4l2_mbus_framefmt new_fmt;
> + struct v4l2_mbus_framefmt *__fmt;
> + unsigned int best_fit = ~0L;
> + unsigned int idx;
> + unsigned int i;
> +
> + mutex_lock(&mt9v111->stream_mutex);
> + if (mt9v111->streaming) {
> + mutex_unlock(&mt9v111->stream_mutex);
> + return -EBUSY;
> + }
> + mutex_unlock(&mt9v111->stream_mutex);
> +
> + if (format->pad)
> + return -EINVAL;
> +
> + /* Update mbus format code and sizes. */
> + for (i = 0; i < ARRAY_SIZE(mt9v111_formats); i++) {
> + if (format->format.code == mt9v111_formats[i].code) {
> + new_fmt.code = mt9v111_formats[i].code;
> + break;
> + }
> + }
> + if (i == ARRAY_SIZE(mt9v111_formats))
> + new_fmt.code = mt9v111_formats[0].code;
> +
> + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) {
> + unsigned int fit = abs(mt9v111_frame_sizes[i].width -
> + format->format.width) +
> + abs(mt9v111_frame_sizes[i].height -
> + format->format.height);
> + if (fit < best_fit) {
> + best_fit = fit;
> + idx = i;
> +
> + if (fit == 0)
> + break;
> + }
> + }
> + new_fmt.width = mt9v111_frame_sizes[idx].width;
> + new_fmt.height = mt9v111_frame_sizes[idx].height;
> +
> + /* Update the device (or pad) format if it has changed. */
> + __fmt = __mt9v111_get_pad_format(mt9v111, cfg, format->pad,
> + format->which);
> +
> + /* Format hasn't changed, stop here. */
> + if (__fmt->code == new_fmt.code &&
> + __fmt->width == new_fmt.width &&
> + __fmt->height == new_fmt.height)
> + goto done;
> +
> + /* Update the format and sizes, then mark changes as pending. */
> + __fmt->code = new_fmt.code;
> + __fmt->width = new_fmt.width;
> + __fmt->height = new_fmt.height;
> +
> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> + mt9v111->pending = true;
> +
> + dev_info(mt9v111->dev, "%s: mbus_code: %x - (%ux%u)\n",
> + __func__, __fmt->code, __fmt->width, __fmt->height);
> +
> +done:
> + format->format = *__fmt;
Same here.
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops mt9v111_core_ops = {
> + .s_power = mt9v111_s_power,
> +};
> +
> +static const struct v4l2_subdev_video_ops mt9v111_video_ops = {
> + .s_stream = mt9v111_s_stream,
> + .s_frame_interval = mt9v111_s_frame_interval,
> + .g_frame_interval = mt9v111_g_frame_interval,
> +};
> +
> +static const struct v4l2_subdev_pad_ops mt9v111_pad_ops = {
> + .enum_mbus_code = mt9v111_enum_mbus_code,
> + .enum_frame_size = mt9v111_enum_frame_size,
> + .enum_frame_interval = mt9v111_enum_frame_interval,
> + .get_fmt = mt9v111_get_format,
> + .set_fmt = mt9v111_set_format,
> +};
> +
> +static const struct v4l2_subdev_ops mt9v111_ops = {
> + .core = &mt9v111_core_ops,
> + .video = &mt9v111_video_ops,
> + .pad = &mt9v111_pad_ops,
> +};
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +static const struct media_entity_operations mt9v111_subdev_entity_ops = {
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static int mt9v111_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd);
> + struct v4l2_mbus_framefmt *__fmt =
> + v4l2_subdev_get_try_format(sd, fh->pad, 0);
> +
> + /* Initialize try format: copy the device format. */
> + *__fmt = mt9v111->fmt;
Please use default configuration, not the current configuration. If this is
all you need in open, I'd suggest to implement the init_cfg pad op instead.
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops mt9v111_internal_ops = {
> + .open = mt9v111_open,
> +};
> +#endif
> +
> +/* --- V4L2 ctrl --- */
> +static int mt9v111_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct mt9v111_dev *mt9v111 = container_of(ctrl->handler,
> + struct mt9v111_dev,
> + ctrls);
> + int ret = 0;
-EINVAL? That way any controls not handled by the function won't be missed.
> +
> + mutex_lock(&mt9v111->pwr_mutex);
> + /*
> + * If sensor is powered down, just cache new control values,
> + * no actual register access.
> + */
> + if (!mt9v111->pwr_count) {
> + mutex_unlock(&mt9v111->pwr_mutex);
> + return 0;
> + }
> + mutex_unlock(&mt9v111->pwr_mutex);
> +
> + switch (ctrl->id) {
> + case V4L2_CID_HBLANK:
> + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE,
> + MT9V111_CORE_R05_HBLANK,
> + MT9V111_CORE_R05_MAX_HBLANK,
> + mt9v111->hblank->val);
> + break;
> + case V4L2_CID_VBLANK:
> + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE,
> + MT9V111_CORE_R06_VBLANK,
> + MT9V111_CORE_R06_MAX_VBLANK,
> + mt9v111->vblank->val);
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops mt9v111_ctrl_ops = {
> + .s_ctrl = mt9v111_s_ctrl,
> +};
> +
> +static int mt9v111_chip_probe(struct mt9v111_dev *mt9v111)
> +{
> + int ret;
> + u16 val;
> +
> + ret = __mt9v111_power_on(&mt9v111->sd);
> + if (ret)
> + return ret;
> +
> + ret = mt9v111_read(mt9v111->client, MT9V111_R01_CORE,
> + MT9V111_CORE_RFF_CHIP_VER, &val);
> + if (ret)
> + return ret;
Power off the device on failure? Same below.
> +
> + if ((val >> 8) != MT9V111_CHIP_ID_HIGH &&
> + (val & 0xff) != MT9V111_CHIP_ID_LOW) {
> + dev_err(mt9v111->dev,
> + "Unable to identify MT9V111 chip: 0x%2x%2x\n",
> + val >> 8, val & 0xff);
> + return ret;
> + }
> +
> + ret = __mt9v111_power_off(&mt9v111->sd);
> + if (ret)
> + return ret;
> +
> + dev_info(mt9v111->dev, "Chip identified: 0x%2x%2x\n",
> + val >> 8, val & 0xff);
> +
> + return 0;
> +}
> +
> +static int mt9v111_probe(struct i2c_client *client)
> +{
> + struct mt9v111_dev *mt9v111;
> + struct v4l2_fract tpf;
> + int ret;
> +
> + mt9v111 = devm_kzalloc(&client->dev, sizeof(*mt9v111), GFP_KERNEL);
> + if (!mt9v111)
> + return -ENOMEM;
> +
> + mt9v111->dev = &client->dev;
> + mt9v111->client = client;
> +
> + mt9v111->clk = devm_clk_get(&client->dev, NULL);
> + if (IS_ERR(mt9v111->clk))
> + return PTR_ERR(mt9v111->clk);
> +
> + mt9v111->sysclk = clk_get_rate(mt9v111->clk);
> + if (mt9v111->sysclk > MT9V111_MAX_CLKIN)
> + return -EINVAL;
> +
> + mt9v111->oe = devm_gpiod_get_optional(&client->dev, "enable",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(mt9v111->oe)) {
> + dev_err(&client->dev, "Unable to get GPIO \"enable\": %ld\n",
> + PTR_ERR(mt9v111->oe));
> + return PTR_ERR(mt9v111->oe);
> + }
> +
> + mt9v111->standby = devm_gpiod_get_optional(&client->dev, "standby",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(mt9v111->standby)) {
> + dev_err(&client->dev, "Unable to get GPIO \"standby\": %ld\n",
> + PTR_ERR(mt9v111->standby));
> + return PTR_ERR(mt9v111->standby);
> + }
> +
> + mt9v111->reset = devm_gpiod_get_optional(&client->dev, "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(mt9v111->reset)) {
> + dev_err(&client->dev, "Unable to get GPIO \"reset\": %ld\n",
> + PTR_ERR(mt9v111->reset));
> + return PTR_ERR(mt9v111->reset);
> + }
> +
> + mutex_init(&mt9v111->pwr_mutex);
> + mutex_init(&mt9v111->stream_mutex);
> + spin_lock_init(&mt9v111->addr_lock);
> +
> + v4l2_ctrl_handler_init(&mt9v111->ctrls, 3);
> +
> + mt9v111->hblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops,
> + V4L2_CID_HBLANK,
> + MT9V111_CORE_R05_MIN_HBLANK,
> + MT9V111_CORE_R05_MAX_HBLANK, 1,
> + MT9V111_CORE_R05_DEF_HBLANK);
> + if (IS_ERR_OR_NULL(mt9v111->hblank)) {
> + ret = PTR_ERR(mt9v111->hblank);
> + goto error_free_ctrls;
> + }
> +
> + mt9v111->vblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops,
> + V4L2_CID_VBLANK,
> + MT9V111_CORE_R06_MIN_VBLANK,
> + MT9V111_CORE_R06_MAX_VBLANK, 1,
> + MT9V111_CORE_R06_DEF_VBLANK);
> + if (IS_ERR_OR_NULL(mt9v111->vblank)) {
> + ret = PTR_ERR(mt9v111->vblank);
> + goto error_free_ctrls;
> + }
> +
> + /* PIXEL_RATE is fixed: just expose it to user space. */
> + v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops,
> + V4L2_CID_PIXEL_RATE, 0,
> + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2), 1,
> + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2));
> +
> + mt9v111->sd.ctrl_handler = &mt9v111->ctrls;
> +
> + /* Start with default configuration: 640x480 UYVY. */
> + mt9v111->fmt.width = 640;
> + mt9v111->fmt.height = 480;
> + mt9v111->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8;
> +
> + /* These are fixed for all supported formats. */
> + mt9v111->fmt.field = V4L2_FIELD_NONE;
> + mt9v111->fmt.colorspace = V4L2_COLORSPACE_SRGB;
> + mt9v111->fmt.ycbcr_enc = V4L2_YCBCR_ENC_601;
> + mt9v111->fmt.quantization = V4L2_QUANTIZATION_LIM_RANGE;
> + mt9v111->fmt.xfer_func = V4L2_XFER_FUNC_SRGB;
> +
> + /* Re-calculate blankings for 640x480@15fps. */
> + mt9v111->fps = 15;
> + tpf.numerator = 1;
> + tpf.denominator = mt9v111->fps;
> + ret = mt9v111_calc_frame_rate(mt9v111, &tpf);
> + if (ret)
> + goto error_free_ctrls;
> +
> + mt9v111->pwr_count = 0;
> + mt9v111->addr_space = MT9V111_R01_IFP;
> + mt9v111->pending = true;
> +
> + v4l2_i2c_subdev_init(&mt9v111->sd, client, &mt9v111_ops);
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> + mt9v111->sd.internal_ops = &mt9v111_internal_ops;
> + mt9v111->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + mt9v111->sd.entity.ops = &mt9v111_subdev_entity_ops;
> + mt9v111->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> + mt9v111->pad.flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&mt9v111->sd.entity, 1, &mt9v111->pad);
> + if (ret)
> + goto error_free_ctrls;
> +#endif
> +
> + ret = mt9v111_chip_probe(mt9v111);
> + if (ret)
> + goto error_free_ctrls;
> +
> + ret = v4l2_async_register_subdev(&mt9v111->sd);
> + if (ret)
> + goto error_free_ctrls;
> +
> + return 0;
> +
> +error_free_ctrls:
> + v4l2_ctrl_handler_free(&mt9v111->ctrls);
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> + media_entity_cleanup(&mt9v111->sd.entity);
> +#endif
> +
> + mutex_destroy(&mt9v111->pwr_mutex);
> + mutex_destroy(&mt9v111->stream_mutex);
> +
> + return ret;
> +}
> +
> +static int mt9v111_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> +
> + v4l2_ctrl_handler_free(&mt9v111->ctrls);
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> + media_entity_cleanup(&sd->entity);
> +#endif
> +
> + mutex_destroy(&mt9v111->pwr_mutex);
> + mutex_destroy(&mt9v111->stream_mutex);
> +
> + devm_gpiod_put(mt9v111->dev, mt9v111->oe);
> + devm_gpiod_put(mt9v111->dev, mt9v111->standby);
> + devm_gpiod_put(mt9v111->dev, mt9v111->reset);
> +
> + devm_clk_put(mt9v111->dev, mt9v111->clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id mt9v111_of_match[] = {
> + { .compatible = "aptina,mt9v111", },
> + { /* sentinel */ },
> +};
> +
> +static struct i2c_driver mt9v111_driver = {
> + .driver = {
> + .name = "mt9v111",
> + .of_match_table = of_match_ptr(mt9v111_of_match),
Please assign this without of_match_ptr().
> + },
> + .probe_new = mt9v111_probe,
> + .remove = mt9v111_remove,
> +};
> +
> +module_i2c_driver(mt9v111_driver);
> +
> +MODULE_DESCRIPTION("V4L2 sensor driver for Aptina MT9V111");
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_LICENSE("GPL v2");
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply
* [PATCH net-next] wan/fsl_ucc_hdlc: use IS_ERR_VALUE() to check return value of qe_muram_alloc
From: YueHaibing @ 2018-07-23 14:12 UTC (permalink / raw)
To: qiang.zhao, davem; +Cc: linux-kernel, netdev, linuxppc-dev, YueHaibing
qe_muram_alloc return a unsigned long integer,which should not
compared with zero. check it using IS_ERR_VALUE() to fix this.
Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wan/fsl_ucc_hdlc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 9b09c9d..5f0366a 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -192,7 +192,7 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
priv->ucc_pram_offset = qe_muram_alloc(sizeof(struct ucc_hdlc_param),
ALIGNMENT_OF_UCC_HDLC_PRAM);
- if (priv->ucc_pram_offset < 0) {
+ if (IS_ERR_VALUE(priv->ucc_pram_offset)) {
dev_err(priv->dev, "Can not allocate MURAM for hdlc parameter.\n");
ret = -ENOMEM;
goto free_tx_bd;
@@ -230,14 +230,14 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
/* Alloc riptr, tiptr */
riptr = qe_muram_alloc(32, 32);
- if (riptr < 0) {
+ if (IS_ERR_VALUE(riptr)) {
dev_err(priv->dev, "Cannot allocate MURAM mem for Receive internal temp data pointer\n");
ret = -ENOMEM;
goto free_tx_skbuff;
}
tiptr = qe_muram_alloc(32, 32);
- if (tiptr < 0) {
+ if (IS_ERR_VALUE(tiptr)) {
dev_err(priv->dev, "Cannot allocate MURAM mem for Transmit internal temp data pointer\n");
ret = -ENOMEM;
goto free_riptr;
--
2.7.0
^ permalink raw reply related
* [PATCH 4/7] x86/umwait_contro: Set global umwait maximum time limit and umwait C0.2 state
From: Fenghua Yu @ 2018-07-23 12:55 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H Peter Anvin
Cc: Ashok Raj, Alan Cox, Ravi V Shankar, linux-kernel, x86,
Fenghua Yu
In-Reply-To: <1532350557-98388-1-git-send-email-fenghua.yu@intel.com>
UMWAIT or TPAUSE called by user process makes processor to reside in
a light-weight power/performance optimized state (C0.1 state) or an
improved power/performance optimized state (C0.2 state).
IA32_UMWAIT_CONTROL MSR register allows OS to set global maximum umwait
time and disable C0.2 on the processor.
The maximum time value in IA32_UMWAIT_CONTROL[31-2] is set as zero which
means there is no global time limit for UMWAIT and TPAUSE instructions.
Each process sets its own umwait maximum time as the instructions operand.
We don't set a non-zero global umwait maximum time value to enforce user
wait timeout because we couldn't find any usage for it.
By default C0.2 is enabled so user wait instructions can enter the state
if user wants to save more power but wakeup time is slower. In some cases
e.g. real time, user wants to disable C0.2 and all C0.2 requests revert
to C0.1.
A new "/sys/devices/system/cpu/umwait_control/umwait_enable_c0_2" file is
created to allow user to check if C0.2 is enabled or disabled and also
allow user to enable or disable C0.2. Value "0" in the file means C0.2 is
disabled. Value "1" means C0.2 is enabled.
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
arch/x86/include/asm/msr-index.h | 4 ++
arch/x86/power/Makefile | 1 +
arch/x86/power/umwait.c | 116 +++++++++++++++++++++++++++++++++++++++
3 files changed, 121 insertions(+)
create mode 100644 arch/x86/power/umwait.c
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 68b2c3150de1..92ef30f0f62d 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -58,6 +58,10 @@
#define MSR_PLATFORM_INFO_CPUID_FAULT_BIT 31
#define MSR_PLATFORM_INFO_CPUID_FAULT BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
+#define MSR_IA32_UMWAIT_CONTROL 0xe1
+#define UMWAIT_CONTROL_C02_BIT 0x0
+#define UMWAIT_CONTROL_C02_MASK 0x00000001
+
#define MSR_PKG_CST_CONFIG_CONTROL 0x000000e2
#define NHM_C3_AUTO_DEMOTE (1UL << 25)
#define NHM_C1_AUTO_DEMOTE (1UL << 26)
diff --git a/arch/x86/power/Makefile b/arch/x86/power/Makefile
index a4701389562c..d3dfa8a47983 100644
--- a/arch/x86/power/Makefile
+++ b/arch/x86/power/Makefile
@@ -8,3 +8,4 @@ CFLAGS_cpu.o := $(nostackp)
obj-$(CONFIG_PM_SLEEP) += cpu.o
obj-$(CONFIG_HIBERNATION) += hibernate_$(BITS).o hibernate_asm_$(BITS).o
+obj-y += umwait.o
diff --git a/arch/x86/power/umwait.c b/arch/x86/power/umwait.c
new file mode 100644
index 000000000000..7fb645c65f22
--- /dev/null
+++ b/arch/x86/power/umwait.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Sysfs interface for umwait control
+ *
+ * Copyright (C) 2018, Intel Corporation.
+ *
+ * Author: Fenghua Yu <fenghua.yu@intel.com>
+ */
+#include <linux/cpu.h>
+#include <asm/msr.h>
+
+static int umwait_enable_c0_2 = 1; /* 0: disable C0.2. 1: enable C0.2. */
+static DEFINE_MUTEX(umwait_lock);
+
+/* Return value that will be used to set umwait control MSR */
+static inline u32 umwait_control_val(void)
+{
+ /*
+ * We do not set global umwait maximum time limit. So bits 31-2 are 0.
+ *
+ * Enable or disable C0.2 (bit 0) based on global setting on all CPUs.
+ * When bit 0 is 1, C0.2 is disabled. Otherwise, C0.2 is enabled.
+ * So value in bit 0 is opposite of umwait_enable_c0_2.
+ */
+ return ~umwait_enable_c0_2 & UMWAIT_CONTROL_C02_MASK;
+}
+
+static ssize_t umwait_enable_c0_2_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", umwait_enable_c0_2);
+}
+
+static ssize_t umwait_enable_c0_2_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int enable_c0_2, cpu, ret;
+ u32 msr_val;
+
+ ret = kstrtou32(buf, 10, &enable_c0_2);
+ if (ret)
+ return ret;
+
+ if (enable_c0_2 != 1 && enable_c0_2 != 0)
+ return -EINVAL;
+
+ mutex_lock(&umwait_lock);
+
+ umwait_enable_c0_2 = enable_c0_2;
+ msr_val = umwait_control_val();
+ get_online_cpus();
+ /* All CPUs have same umwait control setting */
+ for_each_online_cpu(cpu)
+ wrmsr_on_cpu(cpu, MSR_IA32_UMWAIT_CONTROL, msr_val, 0);
+ put_online_cpus();
+
+ mutex_unlock(&umwait_lock);
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(umwait_enable_c0_2);
+
+static struct attribute *umwait_attrs[] = {
+ &dev_attr_umwait_enable_c0_2.attr,
+ NULL
+};
+
+static struct attribute_group umwait_attr_group = {
+ .attrs = umwait_attrs,
+ .name = "umwait_control",
+};
+
+/* Set up umwait control MSR on this CPU using the current global setting. */
+static int umwait_cpu_online(unsigned int cpu)
+{
+ u32 msr_val;
+
+ mutex_lock(&umwait_lock);
+
+ msr_val = umwait_control_val();
+ wrmsr(MSR_IA32_UMWAIT_CONTROL, msr_val, 0);
+
+ mutex_unlock(&umwait_lock);
+
+ return 0;
+}
+
+static int __init umwait_init(void)
+{
+ struct device *dev;
+ int ret;
+
+ if (!boot_cpu_has(X86_FEATURE_WAITPKG))
+ return -ENODEV;
+
+ /* Add CPU global user wait interface to control umwait C0.2 state. */
+ dev = cpu_subsys.dev_root;
+ ret = sysfs_create_group(&dev->kobj, &umwait_attr_group);
+ if (ret)
+ return ret;
+
+ ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "umwait/intel:online",
+ umwait_cpu_online, NULL);
+ if (ret < 0)
+ goto out_group;
+
+ return 0;
+out_group:
+ sysfs_remove_group(&dev->kobj, &umwait_attr_group);
+
+ return ret;
+}
+device_initcall(umwait_init);
--
2.5.0
^ permalink raw reply related
* [PATCH 5/7] x86/vdso: Add vDSO functions for direct store instructions
From: Fenghua Yu @ 2018-07-23 12:55 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H Peter Anvin
Cc: Ashok Raj, Alan Cox, Ravi V Shankar, linux-kernel, x86,
Fenghua Yu
In-Reply-To: <1532350557-98388-1-git-send-email-fenghua.yu@intel.com>
User wants to query if direct store instructions are supported and use
the instructions. The vDSO functions provides fast interface for user
to query the support and use the instructions.
movdiri_supported and its alias __vdso_movdiri_supported check if
movdiri instructions are supported.
movdir64b_supported and its alias __vdso_movdir64b_supported checks
if movdir64b instruction is supported.
movdiri32 and its alias __vdso_movdiri32 provide user APIs for calling
32-bit movdiri instruction.
movdiri64 and its alias __vdso_movdiri64 provide user APIs for calling
64-bit movdiri instruction.
movdir64b and its alias __vdso_movdir64b provide user APIs to move
64-byte data through movdir64b instruction.
The instructions can be implemented in intrinsic functions in future
GCC. But the vDSO interfaces are available to user without the
intrinsic functions support in GCC and the APIs movdiri_supported and
movdir64b_supported cannot be implemented as GCC functions.
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
arch/x86/entry/vdso/Makefile | 2 +-
arch/x86/entry/vdso/vdirectstore.c | 152 +++++++++++++++++++++++++++++++++
arch/x86/entry/vdso/vdso.lds.S | 10 +++
arch/x86/entry/vdso/vma.c | 12 +++
arch/x86/include/asm/vdso_funcs_data.h | 17 ++++
arch/x86/include/asm/vvar.h | 1 +
6 files changed, 193 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/entry/vdso/vdirectstore.c
create mode 100644 arch/x86/include/asm/vdso_funcs_data.h
diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
index b9ed1aa53a26..af4fcae5de83 100644
--- a/arch/x86/entry/vdso/Makefile
+++ b/arch/x86/entry/vdso/Makefile
@@ -17,7 +17,7 @@ VDSO32-$(CONFIG_X86_32) := y
VDSO32-$(CONFIG_IA32_EMULATION) := y
# files to link into the vdso
-vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o
+vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o vdirectstore.o
# files to link into kernel
obj-y += vma.o
diff --git a/arch/x86/entry/vdso/vdirectstore.c b/arch/x86/entry/vdso/vdirectstore.c
new file mode 100644
index 000000000000..6f6d76ed8cde
--- /dev/null
+++ b/arch/x86/entry/vdso/vdirectstore.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * vDSO functions for direct store instructions
+ *
+ * Copyright 2018 Intel Coporation
+ *
+ * Author: Fenghua Yu <fenghua.yu@intel.com>
+ */
+#include <asm/vdso_funcs_data.h>
+
+notrace bool __vdso_movdiri_supported(void)
+{
+ return _vdso_funcs_data->movdiri_supported;
+}
+
+/**
+ * movdiri_supported() - vDSO function for checking movdiri instructions support
+ *
+ * This function checks if direct store instruction movdiri is supported
+ * on this machine.
+ *
+ * movdiri_supported() and its alias __vdso_movdiri_supported() are
+ * implemented as vDSO functions.
+ *
+ * Return:
+ * true: supported
+ *
+ * false: not supported
+ */
+bool movdiri_supported(void)
+ __attribute__((weak, alias("__vdso_movdiri_supported")));
+
+notrace int __vdso_movdir64b_supported(void)
+{
+ return _vdso_funcs_data->movdir64b_supported;
+}
+
+/**
+ * movdir64b_supported() - vDSO function for checking movdir64b instruction
+ * support
+ *
+ * This function checks if direct store instruction movdir64b is supported
+ * on this machine.
+ *
+ * movdir64b_supported() and its alias __vdso_movdir64b_supported() are
+ * implemented as vDSO functions.
+ *
+ * Return:
+ * true: supported
+ *
+ * false: not supported
+ */
+bool movdir64b_supported(void)
+ __attribute__((weak, alias("__vdso_movdir64b_supported")));
+
+notrace int __vdso_movdiri32(int *dst, int data)
+{
+ if (!_vdso_funcs_data->movdiri_supported)
+ return -ENODEV;
+
+ /* movdiri eax, [rdx] */
+ asm volatile(".byte 0x0f, 0x38, 0xf9, 0x02"
+ : "=m" (*dst)
+ : "a" (data), "d" (dst));
+
+ return 0;
+}
+
+/**
+ * movdiri32() - vDSO function for moving doubleword using direct store.
+ * @dst: Destination address.
+ * @data: 32-bit data.
+ *
+ * Moves the doubleword in @data to the destination address @dst
+ * using direct-store operation.
+ *
+ * movdiri32() and its alias __vdso_movdiri32() are implemented as vDSO
+ * functions.
+ *
+ * Return:
+ * 0: Successful
+ *
+ * Less than zero: error code
+ */
+int movdiri32(int *dst, int data)
+ __attribute__((weak, alias("__vdso_movdiri32")));
+
+notrace int __vdso_movdiri64(long *dst, long data)
+{
+ if (!_vdso_funcs_data->movdiri_supported)
+ return -ENODEV;
+
+ /* movdiri rax, [rdx] */
+ asm volatile(".byte 0x48, 0x0f, 0x38, 0xf9, 0x02"
+ : "=m" (*dst)
+ : "a" (data), "d" (dst));
+
+ return 0;
+}
+
+/**
+ * movdiri64() - vDSO function for moving quadword using direct store
+ * @dst: Destination address
+ * @data: 64-bit data
+ *
+ * Moves the quadword in @data to the destination address @dst using
+ * direct-store operation.
+ *
+ * movdiri64() and its alias __vdso_movdiri64() are implemented as vDSO
+ * functions.
+ *
+ * Return:
+ * 0: Successful
+ *
+ * Less than zero: error code
+ */
+void movdiri64(long *dst, long data)
+ __attribute__((weak, alias("__vdso_movdiri64")));
+
+notrace int __vdso_movdir64b(char *dst, char *src)
+{
+ if (!_vdso_funcs_data->movdir64b_supported)
+ return -ENODEV;
+
+ /* movdir64b [rax], rdx */
+ asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
+ : "=m" (*dst)
+ : "a" (src), "d" (dst));
+
+ return 0;
+}
+
+/**
+ * movdir64b() - vDSO function for moving 64 bytes using direct store
+ * @dst: Destination address
+ * @src: Source address
+ *
+ * Moves 64 bytes as direct store with 64 bytes write atomicity from
+ * source memory address @src to destination address @dst.
+ *
+ * @dst must be 64-byte aligned. No alignment requirement for @src.
+ *
+ * movdir64b() and its alias __vdso_movdir64b() are implemented as vDSO
+ * functions.
+ *
+ * Return:
+ * 0: Successful
+ *
+ * Less than zero: error code
+ */
+int movdir64b(char *dst, char *src)
+ __attribute__((weak, alias("__vdso_movdir64b")));
diff --git a/arch/x86/entry/vdso/vdso.lds.S b/arch/x86/entry/vdso/vdso.lds.S
index d3a2dce4cfa9..097cdcda43a5 100644
--- a/arch/x86/entry/vdso/vdso.lds.S
+++ b/arch/x86/entry/vdso/vdso.lds.S
@@ -25,6 +25,16 @@ VERSION {
__vdso_getcpu;
time;
__vdso_time;
+ movdiri_supported;
+ __vdso_movdiri_supported;
+ movdiri32;
+ __vdso_movdiri32;
+ movdiri64;
+ __vdso_movdiri64;
+ movdir64b_supported;
+ __vdso_movdir64b_supported;
+ movdir64b;
+ __vdso_movdir64b;
local: *;
};
}
diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index 5b8b556dbb12..edbe5e63e5c2 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -23,11 +23,14 @@
#include <asm/desc.h>
#include <asm/cpufeature.h>
#include <asm/mshyperv.h>
+#include <asm/vdso_funcs_data.h>
#if defined(CONFIG_X86_64)
unsigned int __read_mostly vdso64_enabled = 1;
#endif
+DEFINE_VVAR(struct vdso_funcs_data, vdso_funcs_data);
+
void __init init_vdso_image(const struct vdso_image *image)
{
BUG_ON(image->size % PAGE_SIZE != 0);
@@ -367,6 +370,14 @@ static int vgetcpu_online(unsigned int cpu)
return smp_call_function_single(cpu, vgetcpu_cpu_init, NULL, 1);
}
+static void __init init_vdso_funcs_data(void)
+{
+ if (static_cpu_has(X86_FEATURE_MOVDIRI))
+ vdso_funcs_data.movdiri_supported = true;
+ if (static_cpu_has(X86_FEATURE_MOVDIR64B))
+ vdso_funcs_data.movdir64b_supported = true;
+}
+
static int __init init_vdso(void)
{
init_vdso_image(&vdso_image_64);
@@ -374,6 +385,7 @@ static int __init init_vdso(void)
#ifdef CONFIG_X86_X32_ABI
init_vdso_image(&vdso_image_x32);
#endif
+ init_vdso_funcs_data();
/* notifier priority > KVM */
return cpuhp_setup_state(CPUHP_AP_X86_VDSO_VMA_ONLINE,
diff --git a/arch/x86/include/asm/vdso_funcs_data.h b/arch/x86/include/asm/vdso_funcs_data.h
new file mode 100644
index 000000000000..b99a5685029e
--- /dev/null
+++ b/arch/x86/include/asm/vdso_funcs_data.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_VDSO_FUNCS_DATA_H
+#define _ASM_X86_VDSO_FUNCS_DATA_H
+
+#include <linux/errno.h>
+#include <linux/types.h>
+#include <asm/vvar.h>
+
+/* This structure contains data used by vDSO functions. */
+struct vdso_funcs_data {
+ bool movdiri_supported; /* if movdiri instruction is supported */
+ bool movdir64b_supported; /* if movdir64b instruction is supported */
+};
+
+#define _vdso_funcs_data (&VVAR(vdso_funcs_data))
+
+#endif /* _ASM_X86_VDSO_FUNCS_DATA_H */
diff --git a/arch/x86/include/asm/vvar.h b/arch/x86/include/asm/vvar.h
index 3f32dfc2ab73..74047c0490fe 100644
--- a/arch/x86/include/asm/vvar.h
+++ b/arch/x86/include/asm/vvar.h
@@ -45,6 +45,7 @@ extern char __vvar_page;
/* DECLARE_VVAR(offset, type, name) */
DECLARE_VVAR(128, struct vsyscall_gtod_data, vsyscall_gtod_data)
+DECLARE_VVAR(256, struct vdso_funcs_data, vdso_funcs_data)
#undef DECLARE_VVAR
--
2.5.0
^ permalink raw reply related
* [PATCH 2/7] x86/cpufeatures: Enumerate MOVDIR64B instruction
From: Fenghua Yu @ 2018-07-23 12:55 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H Peter Anvin
Cc: Ashok Raj, Alan Cox, Ravi V Shankar, linux-kernel, x86,
Fenghua Yu
In-Reply-To: <1532350557-98388-1-git-send-email-fenghua.yu@intel.com>
MOVDIR64B moves 64-bytes as direct-store with 64-bytes write atomicity.
Direct store is implemented by using write combining (WC) for writing
data directly into memory without caching the data.
In low latency offload (e.g. Non-Volatile Memory, etc), MOVDIR64B writes
work descriptors (and data in some cases) to device-hosted work-queues
atomically without cache pollution.
Availability of the MOVDIR64B instruction is indicated by the
presence of the CPUID feature flag MOVDIR64B (CPUID.0x07.0x0:ECX[bit 28]).
Please check the latest Intel Architecture Instruction Set Extensions
and Future Features Programming Reference for more details on the CPUID
feature MOVDIR64B flag.
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
arch/x86/include/asm/cpufeatures.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 92630c469675..69f1137877b6 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -330,6 +330,7 @@
#define X86_FEATURE_RDPID (16*32+22) /* RDPID instruction */
#define X86_FEATURE_CLDEMOTE (16*32+25) /* CLDEMOTE instruction */
#define X86_FEATURE_MOVDIRI (16*32+27) /* MOVDIRI instruction */
+#define X86_FEATURE_MOVDIR64B (16*32+28) /* MOVDIR64B instruction */
/* AMD-defined CPU features, CPUID level 0x80000007 (EBX), word 17 */
#define X86_FEATURE_OVERFLOW_RECOV (17*32+ 0) /* MCA overflow recovery support */
--
2.5.0
^ permalink raw reply related
* Re: [PATCH v3 12/13] x86: add iommu_ops to modify and flush IOMMU mappings
From: Paul Durrant @ 2018-07-23 14:27 UTC (permalink / raw)
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Jan Beulich, Ian Jackson,
xen-devel@lists.xenproject.org
In-Reply-To: <20180723142629.ytcph5yf34pi2vmg@citrix.com>
> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 23 July 2018 15:26
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Jan Beulich <jbeulich@suse.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Julien
> Grall <julien.grall@arm.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim
> (Xen.org) <tim@xen.org>; Wei Liu <wei.liu2@citrix.com>
> Subject: Re: [PATCH v3 12/13] x86: add iommu_ops to modify and flush
> IOMMU mappings
>
> On Tue, Jul 17, 2018 at 02:38:15PM +0100, Paul Durrant wrote:
> [...]
> > +static int iommuop_unmap(struct xen_iommu_op_unmap *op)
> > +{
> [...]
> > +
> > + if ( !iommu_unmap_page(currd, bfn) )
> > + rc = -EIO;
>
> So it is an error to unmap the page successfully? In fact this same line
> is changed in the next patch. I suppose the code here is wrong.
Yes, that looks wrong. I'll check.
Thanks,
Paul
>
> Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* [PATCH 7/7] selftests/vDSO: Add selftest to test vDSO functions for direct store and user wait instructions
From: Fenghua Yu @ 2018-07-23 12:55 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H Peter Anvin
Cc: Ashok Raj, Alan Cox, Ravi V Shankar, linux-kernel, x86,
Fenghua Yu
In-Reply-To: <1532350557-98388-1-git-send-email-fenghua.yu@intel.com>
The selftest tool tests the vDSO functions for calling the instructions
including movdiri32, movdiri64, movdir64b, umonitor, umwait, tpause,
and their support checking.
Limited by testing environment, the selftest doesn't contain some
complex tests e.g. wake up process by writing the monitor address.
After testing environment is ready, the tests will be added.
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
tools/testing/selftests/vDSO/Makefile | 4 +-
tools/testing/selftests/vDSO/vdso_inst_test_x86.c | 405 ++++++++++++++++++++++
2 files changed, 408 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/vDSO/vdso_inst_test_x86.c
diff --git a/tools/testing/selftests/vDSO/Makefile b/tools/testing/selftests/vDSO/Makefile
index f5d7a7851e21..d83228714fbd 100644
--- a/tools/testing/selftests/vDSO/Makefile
+++ b/tools/testing/selftests/vDSO/Makefile
@@ -8,7 +8,7 @@ ifeq ($(CONFIG_X86_32),y)
LDLIBS += -lgcc_s
endif
-TEST_PROGS := $(OUTPUT)/vdso_test $(OUTPUT)/vdso_standalone_test_x86
+TEST_PROGS := $(OUTPUT)/vdso_test $(OUTPUT)/vdso_standalone_test_x86 $(OUTPUT)/vdso_inst_test_x86
all: $(TEST_PROGS)
$(OUTPUT)/vdso_test: parse_vdso.c vdso_test.c
@@ -17,5 +17,7 @@ $(OUTPUT)/vdso_standalone_test_x86: vdso_standalone_test_x86.c parse_vdso.c
vdso_standalone_test_x86.c parse_vdso.c \
-o $@
+$(OUTPUT)/vdso_inst_test_x86: parse_vdso.c vdso_inst_test_x86.c
+
EXTRA_CLEAN := $(TEST_PROGS)
endif
diff --git a/tools/testing/selftests/vDSO/vdso_inst_test_x86.c b/tools/testing/selftests/vDSO/vdso_inst_test_x86.c
new file mode 100644
index 000000000000..157afa7d56a1
--- /dev/null
+++ b/tools/testing/selftests/vDSO/vdso_inst_test_x86.c
@@ -0,0 +1,405 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test vDSO APIs for direct store and user wait instructions
+ *
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Author: Fenghua Yu <fenghua.yu@intel.com>
+ */
+#include <stdint.h>
+#include <elf.h>
+#include <stdio.h>
+#include <sys/auxv.h>
+#include <sys/time.h>
+#include <stdbool.h>
+#include <string.h>
+#include <stdlib.h>
+
+extern void *vdso_sym(const char *version, const char *name);
+extern void vdso_init_from_sysinfo_ehdr(uintptr_t base);
+extern void vdso_init_from_auxv(void *auxv);
+
+const char *kernel_version = "LINUX_2.6";
+
+/*
+ * If the given instruction group is supported.
+ * @vdso_name: vdso function name
+ * @inst_name: instruction group name
+ *
+ * Return:
+ * True: the instruction group is supported
+ * False: the instruction group is not supported
+ */
+bool inst_supported(char *vdso_name, char *inst_name)
+{
+ typedef int (*inst_supported_t)(void);
+ int ret;
+
+ inst_supported_t inst_supported = (inst_supported_t)
+ vdso_sym(kernel_version, vdso_name);
+
+ if (!inst_supported) {
+ printf("Could not find %s\n", vdso_name);
+
+ return false;
+ }
+
+ return inst_supported();
+}
+
+void test_inst_support(char *vdso_name, char *inst_name)
+{
+ int ret;
+
+ ret = inst_supported(vdso_name, inst_name);
+ if (ret)
+ printf("%s supported\n", inst_name);
+ else
+ printf("%s not supported\n", inst_name);
+}
+
+void test_movdiri_support(void)
+{
+ test_inst_support("__vdso_movdiri_supported", "movdiri");
+}
+
+void test_movdir64b_support(void)
+{
+ test_inst_support("__vdso_movdir64b_supported", "movdir64b");
+}
+
+void test_waitpkg_support(void)
+{
+ test_inst_support("__vdso_waitpkg_supported", "waitpkg");
+}
+
+void test_insts_support(void)
+{
+ printf("==");
+ printf("features detection test:\n");
+ test_movdiri_support();
+ test_movdir64b_support();
+ test_waitpkg_support();
+}
+
+/* Test movdiri 32-bit API */
+void test_movdiri32(void)
+{
+ typedef void (*movdiri32_t)(int *, int);
+ char vdso_name[] = "__vdso_movdiri32", inst_name[] = "movdiri32";
+ int ret, data, dst __attribute((aligned(64)));
+
+ printf("==");
+
+ /* movdiri instructions should be supported */
+ ret = inst_supported("__vdso_movdiri_supported", "movdiri");
+ if (!ret) {
+ printf("movdiri not supported\n");
+
+ return;
+ }
+
+ /* movdiri32 API should exist */
+ movdiri32_t movdiri32 = (movdiri32_t)
+ vdso_sym(kernel_version, vdso_name);
+ if (!movdiri32) {
+ printf("Could not find %s\n", vdso_name);
+
+ return;
+ }
+
+ dst = 0;
+ data = 100;
+ /* Call movdiri32 API to move 100 to dst */
+ movdiri32(&dst, data);
+ if (dst == data)
+ printf("%s passes\n", inst_name);
+ else
+ printf("%s fails\n", inst_name);
+}
+
+/* Test movdiri 64-bit API */
+void test_movdiri64(void)
+{
+ typedef void (*movdiri64_t)(long *, long);
+ char vdso_name[] = "__vdso_movdiri64", inst_name[] = "movdiri64";
+ long dst __attribute((aligned(64))), data __attribute((aligned(64)));
+
+ printf("==");
+
+ /* movdiri instructions should be supported */
+ if (!inst_supported("__vdso_movdiri_supported", "movdiri")) {
+ printf("movdiri is not supported\n");
+
+ return;
+ }
+
+ /* movdiri64 API should exist */
+ movdiri64_t movdiri64 = (movdiri64_t)
+ vdso_sym(kernel_version, vdso_name);
+ if (!movdiri64) {
+ printf("Could not find %s\n", vdso_name);
+
+ return;
+ }
+
+ dst = 0;
+ data = 0x123456789abcdef0;
+
+ /* Call movdiri64 API to move 64-bit data to dst */
+ movdiri64(&dst, data);
+
+ if (dst == data)
+ printf("movdiri 64-bit test passed\n");
+ else
+ printf("movdiri 64-bit test failed\n");
+}
+
+void test_movdir64b(void)
+{
+ typedef void (*movdir64b_t)(void *, void *);
+ char __attribute((aligned(64))) dst[1024];
+ char vdso_name[] = "__vdso_movdir64b";
+ char inst_name[] = "movdir64b";
+ int data_size = 64;
+ char src[1024];
+
+ printf("==");
+
+ /* movdiri instructions should be supported */
+ if (!inst_supported("__vdso_movdir64b_supported", "movdir64b")) {
+ printf("movdir64b is not supported\n");
+
+ return;
+ }
+
+ /* movdir64b API should exist */
+ movdir64b_t movdir64b = (movdir64b_t)
+ vdso_sym(kernel_version, vdso_name);
+ if (!movdir64b) {
+ printf("Could not find %s\n", vdso_name);
+
+ return;
+ }
+
+ memset(src, 0, data_size);
+ memset(dst, 0, data_size);
+ for (int i = 0; i < data_size; i++)
+ dst[i] = i;
+
+ /* Call movdir64b API to move 64 bytes data from src to dst */
+ movdir64b(src, dst);
+
+ if (memcmp(src, dst, data_size))
+ printf("movdir64b test failed\n");
+ else
+ printf("movdir64b test passed\n");
+}
+
+bool waitpkg_supported(void)
+{
+ return inst_supported("__vdso_waitpkg_supported", "waitpkg_supported");
+}
+
+bool nsec_to_tsc(unsigned long nsec, unsigned long *tsc)
+{
+ typedef int (*nsec_to_tsc_t)(unsigned long, unsigned long *);
+ char vdso_name[] = "__vdso_nsec_to_tsc";
+ int ret;
+
+ /* nsec_to_tsc API should exist */
+ nsec_to_tsc_t nsec_to_tsc = (nsec_to_tsc_t)
+ vdso_sym(kernel_version, "__vdso_nsec_to_tsc");
+ if (!nsec_to_tsc) {
+ printf("Could not find __vdso_nsec_to_tsc\n");
+
+ return false;
+ }
+
+ /* Call nsec_to_tsc API to convert nsec to tsc */
+ ret = nsec_to_tsc(nsec, tsc);
+ if (ret)
+ return false;
+
+ return true;
+}
+
+static unsigned long rdtsc(void)
+{
+ unsigned int low, high;
+
+ asm volatile ("rdtsc\t\n"
+ : "=a" (low), "=d" (high));
+
+ return (unsigned long)high << 32 | low;
+}
+
+void test_timeout(char *test_name, int state, unsigned long timeout_ns,
+ unsigned long overhead_ns)
+{
+ typedef int (*umwait_t)(int state, unsigned long nsec);
+ typedef int (*tpause_t)(int state, unsigned long nsec);
+ unsigned long tsc1, tsc2, real_tsc, real_ns, tsc_per_nsec;
+ tpause_t tpause;
+ umwait_t umwait;
+
+ if (!nsec_to_tsc(1, &tsc_per_nsec)) {
+ printf("timeout test failed: ns cannot be converted to tsc.\n");
+ return;
+ }
+
+ /* tpause API should exist */
+ tpause = (tpause_t)vdso_sym(kernel_version, "__vdso_tpause");
+ if (!tpause) {
+ printf("Could not find tpause\n");
+
+ return;
+ }
+
+ /* umwait API should exist */
+ umwait = (umwait_t)vdso_sym(kernel_version, "__vdso_umwait");
+ if (!umwait) {
+ printf("Could not find umwait\n");
+
+ return;
+ }
+
+ umwait = (umwait_t)vdso_sym(kernel_version, "umwait");
+ if (!umwait) {
+ printf("Could not find umwait\n");
+
+ return;
+ }
+
+ if (waitpkg_supported()) {
+ if (!strcmp(test_name, "umwait")) {
+ tsc1 = rdtsc();
+ umwait(state, timeout_ns);
+ tsc2 = rdtsc();
+ } else {
+ tsc1 = rdtsc();
+ tpause(state, timeout_ns);
+ tsc2 = rdtsc();
+ }
+ real_tsc = tsc2 - tsc1;
+ real_ns = real_tsc / tsc_per_nsec;
+ /* Give enough time for overhead on slow running machine. */
+ if (abs(real_ns - timeout_ns) < overhead_ns) {
+ printf("%s C0.%1d test passed\n", test_name, state + 1);
+ } else {
+ printf("%s test failed:\n", test_name);
+ printf("real=%luns, expected=%luns. ",
+ real_ns, timeout_ns);
+ printf("Likely due to slow machine. ");
+ printf("Please adjust overhead_ns or re-run test for ");
+ printf("a few more times.\n");
+ }
+ } else {
+ printf("%s is not supported\n", test_name);
+ }
+}
+
+void test_tpause_timeout(int state)
+{
+ /*
+ * Timeout 100usec. Assume overhead of executing umwait is 30usec.
+ * You can adjust the overhead number based on your machine.
+ */
+ test_timeout("tpause", state, 100000, 30000);
+}
+
+/* Test tpause API */
+void test_tpause(void)
+{
+ printf("==");
+ /* Test timeout in state 0 (C0.2). */
+ test_tpause_timeout(0);
+ /* Test timeout in state 1 (C0.1). */
+ test_tpause_timeout(1);
+ /* More tests ... */
+}
+
+char umonitor_range[1024];
+
+void test_umonitor_only(void)
+{
+ typedef void (*umonitor_t)(void *addr);
+
+ /* umonitor API should exist */
+ umonitor_t umonitor = (umonitor_t)
+ vdso_sym(kernel_version, "__vdso_umonitor");
+ if (!umonitor) {
+ printf("Could not find umonitor\n");
+
+ return;
+ }
+
+ if (waitpkg_supported()) {
+ umonitor(umonitor_range);
+ printf("umonitor test passed\n");
+ } else {
+ printf("waitpkg not supported\n");
+ }
+}
+
+/* Test umonitor API */
+void test_umonitor(void)
+{
+ printf("==");
+ test_umonitor_only();
+}
+
+void test_umwait_timeout(int state)
+{
+ /*
+ * Timeout 100usec. Overhead of executing umwait assumes 90usec.
+ * You can adjust the overhead number based on your machine.
+ */
+ test_timeout("umwait", state, 100000, 90000);
+}
+
+/* Test umwait API */
+void test_umwait(void)
+{
+ printf("==");
+ /* Test timeout in state 0 (C0.2). */
+ test_umwait_timeout(0);
+ /* Test timeout in state 1 (C0.1). */
+ test_umwait_timeout(1);
+ /* To add more tests here ... */
+}
+
+void show_basic_info(void)
+{
+ unsigned long tsc;
+ int ret;
+
+ ret = nsec_to_tsc(1, &tsc);
+ if (ret < 0)
+ printf("not tsc freq CPUID available\n");
+ else
+ printf("1 nsec = %lu tsc\n", tsc);
+}
+
+int main(int argc, char **argv)
+{
+ unsigned long sysinfo_ehdr = getauxval(AT_SYSINFO_EHDR);
+
+ if (!sysinfo_ehdr) {
+ printf("AT_SYSINFO_EHDR is not present!\n");
+ return 0;
+ }
+
+ vdso_init_from_sysinfo_ehdr(getauxval(AT_SYSINFO_EHDR));
+
+ show_basic_info();
+ test_insts_support();
+ test_movdiri32();
+ test_movdiri64();
+ test_movdir64b();
+ test_umonitor();
+ test_umwait();
+ test_tpause();
+
+ return 0;
+}
--
2.5.0
^ permalink raw reply related
* [PATCH 6/7] x86/vdso: Add vDSO functions for user wait instructions
From: Fenghua Yu @ 2018-07-23 12:55 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H Peter Anvin
Cc: Ashok Raj, Alan Cox, Ravi V Shankar, linux-kernel, x86,
Fenghua Yu
In-Reply-To: <1532350557-98388-1-git-send-email-fenghua.yu@intel.com>
User wants to query if user wait instructions (umonitor, umwait, and
tpause) are supported and use the instructions. The vDSO functions
provides fast interface for user to check the support and use the
instructions.
waitpkg_supported and its alias __vdso_waitpkg_supported check if
user wait instructions (a.k.a. wait package feature) are supported
umonitor and its alias __vdso_umonitor provide user APIs for calling
umonitor instruction.
umwait and its alias __vdso_umwait provide user APIs for calling
umwait instruction.
tpause and its alias __vdso_tpause provide user APIs for calling
tpause instruction.
nsec_to_tsc and its alias __vdso_nsec_to_tsc converts nanoseconds
to TSC counter if TSC frequency is known. It will fail if TSC frequency
is unknown.
The instructions can be implemented in intrinsic functions in future
GCC. But the vDSO interfaces are available to user without the
intrinsic functions support in GCC and the API waitpkg_supported and
nsec_to_tsc cannot be implemented as GCC functions.
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
arch/x86/entry/vdso/Makefile | 2 +-
arch/x86/entry/vdso/vdso.lds.S | 10 ++
arch/x86/entry/vdso/vma.c | 9 ++
arch/x86/entry/vdso/vuserwait.c | 233 +++++++++++++++++++++++++++++++++
arch/x86/include/asm/vdso_funcs_data.h | 3 +
5 files changed, 256 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/entry/vdso/vuserwait.c
diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
index af4fcae5de83..fb0062b09b3c 100644
--- a/arch/x86/entry/vdso/Makefile
+++ b/arch/x86/entry/vdso/Makefile
@@ -17,7 +17,7 @@ VDSO32-$(CONFIG_X86_32) := y
VDSO32-$(CONFIG_IA32_EMULATION) := y
# files to link into the vdso
-vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o vdirectstore.o
+vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o vdirectstore.o vuserwait.o
# files to link into kernel
obj-y += vma.o
diff --git a/arch/x86/entry/vdso/vdso.lds.S b/arch/x86/entry/vdso/vdso.lds.S
index 097cdcda43a5..0942710608bf 100644
--- a/arch/x86/entry/vdso/vdso.lds.S
+++ b/arch/x86/entry/vdso/vdso.lds.S
@@ -35,6 +35,16 @@ VERSION {
__vdso_movdir64b_supported;
movdir64b;
__vdso_movdir64b;
+ waitpkg_supported;
+ __vdso_waitpkg_supported;
+ umonitor;
+ __vdso_umonitor;
+ umwait;
+ __vdso_umwait;
+ tpause;
+ __vdso_tpause;
+ nsec_to_tsc;
+ __vdso_nsec_to_tsc;
local: *;
};
}
diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index edbe5e63e5c2..006dfb5e5003 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -372,10 +372,19 @@ static int vgetcpu_online(unsigned int cpu)
static void __init init_vdso_funcs_data(void)
{
+ struct system_counterval_t sys_counterval;
+
if (static_cpu_has(X86_FEATURE_MOVDIRI))
vdso_funcs_data.movdiri_supported = true;
if (static_cpu_has(X86_FEATURE_MOVDIR64B))
vdso_funcs_data.movdir64b_supported = true;
+ if (static_cpu_has(X86_FEATURE_WAITPKG))
+ vdso_funcs_data.waitpkg_supported = true;
+ if (static_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)) {
+ vdso_funcs_data.tsc_known_freq = true;
+ sys_counterval = convert_art_ns_to_tsc(1);
+ vdso_funcs_data.tsc_per_nsec = sys_counterval.cycles;
+ }
}
static int __init init_vdso(void)
diff --git a/arch/x86/entry/vdso/vuserwait.c b/arch/x86/entry/vdso/vuserwait.c
new file mode 100644
index 000000000000..17ff564aef7e
--- /dev/null
+++ b/arch/x86/entry/vdso/vuserwait.c
@@ -0,0 +1,233 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * vDSO functions for user wait instructions
+ *
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Author: Fenghua Yu <fenghua.yu@intel.com>
+ */
+#include <linux/timer.h>
+#include <asm/vdso_funcs_data.h>
+
+notrace bool __vdso_waitpkg_supported(void)
+{
+ return _vdso_funcs_data->waitpkg_supported;
+}
+
+/**
+ * waitpkg_supported() - vDSO function for checking if user wait instructions
+ * are supported
+ *
+ * waitpkg instructions (a.k.a. user wait instructions) group has three
+ * instructions: umonitor, umwait, and tpause. This vDSO API tells user
+ * if the instructions group is supported on the machine.
+ *
+ * waitpkg_supported() and its alias __vdso_waitpkg_supported() are implemented
+ * as vDSO functions.
+ *
+ * Return:
+ * true: supported
+ *
+ * false: not supported
+ */
+bool waitpkge_supported(void)
+ __attribute__((weak, alias("__vdso_waitpkg_supported")));
+
+notrace int __vdso_nsec_to_tsc(unsigned long nsec, unsigned long *tsc)
+{
+ if (!_vdso_funcs_data->tsc_known_freq)
+ return -ENODEV;
+
+ *tsc = _vdso_funcs_data->tsc_per_nsec * nsec;
+
+ return 0;
+}
+
+/**
+ * nsec_to_tsc() - vDSO function for converting nanoseconds to TSC counter
+ * @nsec: nanoseconds
+ * @tsc: Returned TSC counter
+ *
+ * If TSC has known frequency (i.e. X86_FEATURE_TSC_KNOWN_FREQ is supported),
+ * convert nanoseconds to TSC counter.
+ *
+ * nsec_to_tsc() and its alias __vdso_nsec_to_tsc() are implemented
+ * as vDSO functions.
+ *
+ * Return:
+ * 0: Successful
+ *
+ * Less than zero: error code
+ */
+int nsec_to_tsc(unsigned long nsec, unsigned long *tsc)
+ __attribute__((weak, alias("__vdso_nsec_to_tsc")));
+
+notrace int __vdso_umonitor(void *addr)
+{
+ if (!_vdso_funcs_data->waitpkg_supported)
+ return -ENODEV;
+
+ asm volatile("mov %0, %%rdi\t\n"
+ ".byte 0xf3, 0x0f, 0xae, 0xf7\t\n"
+ : : "r" (addr));
+
+ return 0;
+}
+
+/**
+ * umonitor() - vDSO function for setting up monitoring address
+ * @addr: Monitored address
+ *
+ * The vDSO function sets up address monitoring hardware using address @addr.
+ * It can be executed at any privilege level.
+ *
+ * umonitor() and its alias __vdso_umonitor() are implemented
+ * as vDSO functions.
+ *
+ * Return:
+ * 0: Successful
+ *
+ * Less than zero: error code
+ */
+int umonitor(void *addr)
+ __attribute__((weak, alias("__vdso_umonitor")));
+
+static inline int _umwait(int state, unsigned long eax, unsigned long edx)
+{
+ unsigned long cflags;
+
+ asm volatile("mov %3, %%edi\t\n"
+ ".byte 0xf2, 0x0f, 0xae, 0xf7\t\n"
+ "pushf\t\n"
+ "pop %0\t\n"
+ : "=r" (cflags)
+ : "d" (edx), "a" (eax), "r"(state));
+
+ /*
+ * If the processor wakes due to expiration of OS time-limit, the CF
+ * flag is set. Otherwise, the flag is cleared.
+ */
+ return cflags & 1;
+}
+
+notrace int __vdso_umwait(int state, unsigned long nsec)
+{
+ unsigned long tsc;
+ int ret;
+
+ if (!_vdso_funcs_data->waitpkg_supported)
+ return -ENODEV;
+
+ if (state != 0 && state != 1)
+ return -EINVAL;
+
+ ret = nsec_to_tsc(nsec, &tsc);
+ if (ret)
+ return ret;
+
+ /* Get umwait deadline */
+ tsc += rdtsc();
+ ret = _umwait(state, tsc & 0xffffffff, tsc >> 32);
+
+ return ret;
+}
+
+/**
+ * umwait() - vDSO function for user monitor wait
+ * @state: State
+ * @nsec: Time out in nanoseconds
+ *
+ * A hint that allows the processor to stop instruction execution and
+ * enter an implementation-dependent optimized state. The processor
+ * wakes up because of events such as store to the monitored address,
+ * timeout, NMI, SMI, machine check, debug exception, etc.
+ *
+ * State 0 is light-weight power optimized state. It allows the processor
+ * to enter C0.2 state which has larger power saving but slower wakeup time.
+ *
+ * State 1 is performance optimized state. It allows the processor
+ * to enter C0.1 state which has smaller power saving but faster wakeup time.
+ *
+ * This function can be executed at any privilege level.
+ *
+ * umwait() and its alias __vdso_umwait() are implemented as vDSO functions.
+ *
+ * Return:
+ * 1: the processor wakes due to expiration of OS time-limit
+ *
+ * 0: the processor wakes due to other reasons
+ *
+ * less than 0: error code
+ */
+int umwait(int state, unsigned long nsec)
+ __attribute__((weak, alias("__vdso_umwait")));
+
+static inline int _tpause(int state, unsigned long eax, unsigned long edx)
+{
+ unsigned long cflags;
+
+ asm volatile("mov %3, %%edi\t\n"
+ ".byte 0x66, 0x0f, 0xae, 0xf7\t\n"
+ "pushf\t\n"
+ "pop %0\t\n"
+ : "=r" (cflags)
+ : "d" (edx), "a" (eax), "r"(state));
+
+ /*
+ * If the processor wakes due to expiration of OS time-limit, the CF
+ * flag is set. Otherwise, the flag is cleared.
+ */
+ return cflags & 1;
+}
+
+notrace int __vdso_tpause(int state, unsigned long nsec)
+{
+ unsigned long tsc;
+ int ret;
+
+ if (!_vdso_funcs_data->waitpkg_supported)
+ return -ENODEV;
+
+ if (state != 0 && state != 1)
+ return -EINVAL;
+
+ ret = nsec_to_tsc(nsec, &tsc);
+ if (ret)
+ return ret;
+
+ /* Get tpause deadline */
+ tsc += rdtsc();
+ ret = _tpause(state, tsc & 0xffffffff, tsc >> 32);
+
+ return ret;
+}
+
+/**
+ * tpause() - vDSO function for timed pause
+ * @state: State
+ * @nsec: Timeout in nanoseconds
+ *
+ * tpause() allows the processor to stop instruction execution and
+ * enter an implementation-dependent optimized state. The processor
+ * wakes up because of events such as store to the monitored
+ * address, timeout, NMI, SMI, machine check, debug exception, etc.
+ *
+ * State 0 is light-weight power optimized state. It allows the processor
+ * to enter C0.2 state which has larger power saving but slower wakeup time.
+ *
+ * State 1 is performance optimized state. It allows the processor
+ * to enter C0.1 state which has smaller power saving but faster wakeup time.
+ *
+ * This function can be executed at any privilege level.
+ *
+ * tpause() and its alias __vdso_tpause() are implemented as vDSO functions.
+ *
+ * Return:
+ * 1: the processor wakes due to expiration of OS time-limit
+ *
+ * 0: the processor wakes due to other reasons
+ *
+ * less than 0: error code
+ */
+int tpause(int state, unsigned long nsec)
+ __attribute__((weak, alias("__vdso_tpause")));
diff --git a/arch/x86/include/asm/vdso_funcs_data.h b/arch/x86/include/asm/vdso_funcs_data.h
index b99a5685029e..a4caa64bbe8d 100644
--- a/arch/x86/include/asm/vdso_funcs_data.h
+++ b/arch/x86/include/asm/vdso_funcs_data.h
@@ -10,6 +10,9 @@
struct vdso_funcs_data {
bool movdiri_supported; /* if movdiri instruction is supported */
bool movdir64b_supported; /* if movdir64b instruction is supported */
+ bool waitpkg_supported; /* if wait pkg instructions are supported */
+ bool tsc_known_freq; /* if TSC has known freqency */
+ u64 tsc_per_nsec; /* TSC counter per nanosecond */
};
#define _vdso_funcs_data (&VVAR(vdso_funcs_data))
--
2.5.0
^ permalink raw reply related
* [PATCH 3/7] x86/cpufeatures: Enumerate UMONITOR, UMWAIT, and TPAUSE instructions
From: Fenghua Yu @ 2018-07-23 12:55 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H Peter Anvin
Cc: Ashok Raj, Alan Cox, Ravi V Shankar, linux-kernel, x86,
Fenghua Yu
In-Reply-To: <1532350557-98388-1-git-send-email-fenghua.yu@intel.com>
UMONITOR, UMWAIT, and TPAUSE are a set of user wait instructions.
UMONITOR arms address monitoring hardware using an address. A store
to an address within the specified address range triggers the
monitoring hardware to wake up the processor waiting in umwait.
UMWAIT instructs the processor to enter an implementation-dependent
optimized state while monitoring a range of addresses. The optimized
state may be either a light-weight power/performance optimized state
(c0.1 state) or an improved power/performance optimized state
(c0.2 state).
The UMONITOR and UMWAIT operate together to provide power saving
in idle.
TPAUSE instructs the processor to enter an implementation-dependent
optimized state c0.1 or c0.2 state and wake up when time-stamp counter
reaches specified timeout.
The three instructions may be executed at any privilege level.
Availability of the user wait instructions is indicated by the presence
of the CPUID feature flag WAITPKG CPUID.0x07.0x0:ECX[5].
Please check the latest Intel Architecture Instruction Set Extensions
and Future Features Programming Reference for more details on the
instructions and CPUID feature WAITPKG flag.
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
arch/x86/include/asm/cpufeatures.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 69f1137877b6..70ed3087821d 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -318,6 +318,7 @@
#define X86_FEATURE_UMIP (16*32+ 2) /* User Mode Instruction Protection */
#define X86_FEATURE_PKU (16*32+ 3) /* Protection Keys for Userspace */
#define X86_FEATURE_OSPKE (16*32+ 4) /* OS Protection Keys Enable */
+#define X86_FEATURE_WAITPKG (16*32+ 5) /* UMONITOR/UMWAIT/TPAUSE Instructions */
#define X86_FEATURE_AVX512_VBMI2 (16*32+ 6) /* Additional AVX512 Vector Bit Manipulation Instructions */
#define X86_FEATURE_GFNI (16*32+ 8) /* Galois Field New Instructions */
#define X86_FEATURE_VAES (16*32+ 9) /* Vector AES */
--
2.5.0
^ permalink raw reply related
* [PATCH 1/7] x86/cpufeatures: Enumerate MOVDIRI instruction
From: Fenghua Yu @ 2018-07-23 12:55 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H Peter Anvin
Cc: Ashok Raj, Alan Cox, Ravi V Shankar, linux-kernel, x86,
Fenghua Yu
In-Reply-To: <1532350557-98388-1-git-send-email-fenghua.yu@intel.com>
MOVDIRI moves doubleword or quadword from register to memory through
direct store which is implemented by using write combining (WC) for
writing data directly into memory without caching the data.
Programmable agents can handle streaming offload (e.g. high speed packet
processing in network). Hardware implements a doorbell (tail pointer)
register that is updated by software when adding new work-elements to
the streaming offload work-queue.
MOVDIRI can be used as the doorbell write which is a 4-byte or 8-byte
uncachable write to MMIO. MOVDIRI has lower overhead than other ways
to write the doorbell.
Availability of the MOVDIRI instruction is indicated by the presence of
the CPUID feature flag MOVDIRI(CPUID.0x07.0x0:ECX[bit 27]).
Please check the latest Intel Architecture Instruction Set Extensions
and Future Features Programming Reference for more details on the CPUID
feature MOVDIRI flag.
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
arch/x86/include/asm/cpufeatures.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 5701f5cecd31..92630c469675 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -329,6 +329,7 @@
#define X86_FEATURE_LA57 (16*32+16) /* 5-level page tables */
#define X86_FEATURE_RDPID (16*32+22) /* RDPID instruction */
#define X86_FEATURE_CLDEMOTE (16*32+25) /* CLDEMOTE instruction */
+#define X86_FEATURE_MOVDIRI (16*32+27) /* MOVDIRI instruction */
/* AMD-defined CPU features, CPUID level 0x80000007 (EBX), word 17 */
#define X86_FEATURE_OVERFLOW_RECOV (17*32+ 0) /* MCA overflow recovery support */
--
2.5.0
^ permalink raw reply related
* [PATCH 0/7] x86: Enable a few new instructions
From: Fenghua Yu @ 2018-07-23 12:55 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H Peter Anvin
Cc: Ashok Raj, Alan Cox, Ravi V Shankar, linux-kernel, x86,
Fenghua Yu
A few new instructions including direct stores (movdiri and movdir64b)
and user wait (umwait, umonitor, and tpause) and IA32_MWAIT_CONTROL MSR to
control umwait/umonitor/tpause behaviors will be available in Tremont and
other future x86 processors.
This patch set enumerates the instructions, adds a sysfs interface for
user to configure the umwait/umonitor/tpause instructions, and provides
APIs for user to call the instructions.
The sysfs interface file are in /sys/devices/system/cpu/umwait_control/
umwait_enable_c0_2 because it's hard to find an existing place to host
the files.
The user APIs for the instructions are implemented as vDSO functions.
Detailed information on the instructions and the MSR can be found in
the latest Intel Architecture Instruction Set Extensions and Future
Features Programming Reference at
https://software.intel.com/sites/default/files/managed/c5/15/architecture-instruction-set-extensions-programming-reference.pdf
Changelog:
Based on comments from Thomas:
- Change user APIs to vDSO functions
- Change sysfs to positive logic and enable file name
- Change patch descriptions etc
Fenghua Yu (7):
x86/cpufeatures: Enumerate MOVDIRI instruction
x86/cpufeatures: Enumerate MOVDIR64B instruction
x86/cpufeatures: Enumerate UMONITOR, UMWAIT, and TPAUSE instructions
x86/umwait_contro: Set global umwait maximum time limit and umwait
C0.2 state
x86/vdso: Add vDSO functions for direct store instructions
x86/vdso: Add vDSO functions for user wait instructions
selftests/vDSO: Add selftest to test vDSO functions for direct store
and user wait instructions
arch/x86/entry/vdso/Makefile | 2 +-
arch/x86/entry/vdso/vdirectstore.c | 152 ++++++++
arch/x86/entry/vdso/vdso.lds.S | 20 ++
arch/x86/entry/vdso/vma.c | 21 ++
arch/x86/entry/vdso/vuserwait.c | 233 +++++++++++++
arch/x86/include/asm/cpufeatures.h | 3 +
arch/x86/include/asm/msr-index.h | 4 +
arch/x86/include/asm/vdso_funcs_data.h | 20 ++
arch/x86/include/asm/vvar.h | 1 +
arch/x86/power/Makefile | 1 +
arch/x86/power/umwait.c | 116 +++++++
tools/testing/selftests/vDSO/Makefile | 4 +-
tools/testing/selftests/vDSO/vdso_inst_test_x86.c | 405 ++++++++++++++++++++++
13 files changed, 980 insertions(+), 2 deletions(-)
create mode 100644 arch/x86/entry/vdso/vdirectstore.c
create mode 100644 arch/x86/entry/vdso/vuserwait.c
create mode 100644 arch/x86/include/asm/vdso_funcs_data.h
create mode 100644 arch/x86/power/umwait.c
create mode 100644 tools/testing/selftests/vDSO/vdso_inst_test_x86.c
--
2.5.0
^ permalink raw reply
* Re: [PATCH v2 00/16] i.MX media mem2mem scaler
From: Philipp Zabel @ 2018-07-23 13:26 UTC (permalink / raw)
To: Steve Longerbeam, linux-media; +Cc: Nicolas Dufresne, kernel
In-Reply-To: <38565a74-7c79-1af6-6ed6-b44a20c9266c@gmail.com>
Hi Steve,
On Sun, 2018-07-22 at 11:30 -0700, Steve Longerbeam wrote:
[...]
> To aid in debugging this I created branch 'imx-mem2mem.stevel' in my
> mediatree fork on github. I moved the mem2mem driver to the beginning
> and added a few patches:
>
> d317a7771c ("gpu: ipu-cpmem: add WARN_ON_ONCE() for unaligned dma buffers")
> b4362162c0 ("media: imx: mem2mem: Use ipu_image_convert_adjust in try
> format")
> 4758be0cf8 ("gpu: ipu-v3: image-convert: Fix width/height alignment")
> d069163c7f ("gpu: ipu-v3: image-convert: Fix input bytesperline clamp in
> adjust")
>
> (feel free to squash some of those if you agree with them for v3).
Thank you, I've squashed them where it made sense:
- "media: imx: mem2mem: Use ipu_image_convert_adjust in try format"
into "media: imx: add mem2mem device" so it could be merged
independently,
- "gpu: ipu-v3: image-convert: Fix width/height alignment" into
"gpu: ipu-v3: image-convert: relax alignment restrictions", which
itself is squashed together from "gpu: ipu-v3: image-convert: relax
input alignment restrictions" and "gpu: ipu-v3: image-convert: relax
output alignment restrictions", and
- "gpu: ipu-v3: image-convert: Fix input bytesperline clamp in adjust"
into "gpu: ipu-v3: image-convert: fix bytesperline adjustment".
I've added some fixes and limited output tile top/left alignment to 8x8
IRT block size if the rotator is being used, and dropped the current
state into this branch:
git://git.pengutronix.de/pza/linux imx-mem2mem
regards
Philipp
^ permalink raw reply
* Re: [PATCH v3 12/13] x86: add iommu_ops to modify and flush IOMMU mappings
From: Wei Liu @ 2018-07-23 14:26 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, xen-devel
In-Reply-To: <20180717133816.37958-13-paul.durrant@citrix.com>
On Tue, Jul 17, 2018 at 02:38:15PM +0100, Paul Durrant wrote:
[...]
> +static int iommuop_unmap(struct xen_iommu_op_unmap *op)
> +{
[...]
> +
> + if ( !iommu_unmap_page(currd, bfn) )
> + rc = -EIO;
So it is an error to unmap the page successfully? In fact this same line
is changed in the next patch. I suppose the code here is wrong.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* Re: [PATCH 0/7] fs_info cleanups for volume.c
From: David Sterba @ 2018-07-23 13:25 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
In-Reply-To: <1532104673-28311-1-git-send-email-nborisov@suse.com>
On Fri, Jul 20, 2018 at 07:37:46PM +0300, Nikolay Borisov wrote:
> Here are a bunch of patches which cleanup extraneous fs_info parameters to
> function which already take a structure that holds a reference to the fs_info.
>
> Except for patches 4 and 5, everything else is correct - due to those functions
> always taking a transaction. 4 and 5 in turn reference the fs_info from
> struct btrfs_device. Inspecting the callers I managed to convince myself that
> those function are always called with well-formed btrfs_device i.e one which
> has its fs_info member initialised. Reviewers might want to pay extra
> attention to that but otherwise they are trivial.
4 and 5 look good to me, a device without a valid fs_info has a short
timespan and should not appear anywhere besides the helpers that set up
fs_devices etc. Series added to misc-next, thanks.
^ permalink raw reply
* [stable-4.14 22/23] blk-mq: quiesce queue during switching io sched and updating nr_requests
From: Jack Wang @ 2018-07-23 13:24 UTC (permalink / raw)
To: gregkh, stable; +Cc: Ming Lei, Jens Axboe, Jack Wang
In-Reply-To: <1532352285-13589-1-git-send-email-jinpuwang@gmail.com>
From: Ming Lei <ming.lei@redhat.com>
commit 24f5a90f0d13a97b51aa79f468143fafea4246bb upstream
Dispatch may still be in-progress after queue is frozen, so we have to
quiesce queue before switching IO scheduler and updating nr_requests.
Also when switching io schedulers, blk_mq_run_hw_queue() may still be
called somewhere(such as from nvme_reset_work()), and io scheduler's
per-hctx data may not be setup yet, so cause oops even inside
blk_mq_hctx_has_pending(), such as it can be run just between:
ret = e->ops.mq.init_sched(q, e);
AND
ret = e->ops.mq.init_hctx(hctx, i)
inside blk_mq_init_sched().
This reverts commit 7a148c2fcff8330(block: don't call blk_mq_quiesce_queue()
after queue is frozen) basically, and makes sure blk_mq_hctx_has_pending
won't be called if queue is quiesced.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Fixes: 7a148c2fcff83309(block: don't call blk_mq_quiesce_queue() after queue is frozen)
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Tested-by: Yi Zhang <yi.zhang@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
block/blk-mq.c | 27 ++++++++++++++++++++++++++-
block/elevator.c | 2 ++
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index b3f15d9c7c5a..7d396b77e5e8 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1263,7 +1263,30 @@ EXPORT_SYMBOL(blk_mq_delay_run_hw_queue);
bool blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
{
- if (blk_mq_hctx_has_pending(hctx)) {
+ int srcu_idx;
+ bool need_run;
+
+ /*
+ * When queue is quiesced, we may be switching io scheduler, or
+ * updating nr_hw_queues, or other things, and we can't run queue
+ * any more, even __blk_mq_hctx_has_pending() can't be called safely.
+ *
+ * And queue will be rerun in blk_mq_unquiesce_queue() if it is
+ * quiesced.
+ */
+ if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
+ rcu_read_lock();
+ need_run = !blk_queue_quiesced(hctx->queue) &&
+ blk_mq_hctx_has_pending(hctx);
+ rcu_read_unlock();
+ } else {
+ srcu_idx = srcu_read_lock(hctx->queue_rq_srcu);
+ need_run = !blk_queue_quiesced(hctx->queue) &&
+ blk_mq_hctx_has_pending(hctx);
+ srcu_read_unlock(hctx->queue_rq_srcu, srcu_idx);
+ }
+
+ if (need_run) {
__blk_mq_delay_run_hw_queue(hctx, async, 0);
return true;
}
@@ -2710,6 +2733,7 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
return -EINVAL;
blk_mq_freeze_queue(q);
+ blk_mq_quiesce_queue(q);
ret = 0;
queue_for_each_hw_ctx(q, hctx, i) {
@@ -2734,6 +2758,7 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
if (!ret)
q->nr_requests = nr;
+ blk_mq_unquiesce_queue(q);
blk_mq_unfreeze_queue(q);
return ret;
diff --git a/block/elevator.c b/block/elevator.c
index 153926a90901..229eb4b5604c 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -951,6 +951,7 @@ static int elevator_switch_mq(struct request_queue *q,
int ret;
blk_mq_freeze_queue(q);
+ blk_mq_quiesce_queue(q);
if (q->elevator) {
if (q->elevator->registered)
@@ -977,6 +978,7 @@ static int elevator_switch_mq(struct request_queue *q,
blk_add_trace_msg(q, "elv switch: none");
out:
+ blk_mq_unquiesce_queue(q);
blk_mq_unfreeze_queue(q);
return ret;
}
--
2.7.4
^ permalink raw reply related
* [stable-4.14 23/23] scsi: core: run queue if SCSI device queue isn't ready and queue is idle
From: Jack Wang @ 2018-07-23 13:24 UTC (permalink / raw)
To: gregkh, stable; +Cc: Ming Lei, Martin K . Petersen, Jack Wang
In-Reply-To: <1532352285-13589-1-git-send-email-jinpuwang@gmail.com>
From: Ming Lei <ming.lei@redhat.com>
commit 7e70aa789d4a0c89dbfbd2c8a974a4df717475ec upstream
Before commit 0df21c86bdbf ("scsi: implement .get_budget and .put_budget
for blk-mq"), we run queue after 3ms if queue is idle and SCSI device
queue isn't ready, which is done in handling BLK_STS_RESOURCE. After
commit 0df21c86bdbf is introduced, queue won't be run any more under
this situation.
IO hang is observed when timeout happened, and this patch fixes the IO
hang issue by running queue after delay in scsi_dev_queue_ready, just
like non-mq. This issue can be triggered by the following script[1].
There is another issue which can be covered by running idle queue: when
.get_budget() is called on request coming from hctx->dispatch_list, if
one request just completes during .get_budget(), we can't depend on
SCSI's restart to make progress any more. This patch fixes the race too.
With this patch, we basically recover to previous behaviour (before
commit 0df21c86bdbf) of handling idle queue when running out of
resource.
[1] script for test/verify SCSI timeout
rmmod scsi_debug
modprobe scsi_debug max_queue=1
DEVICE=`ls -d /sys/bus/pseudo/drivers/scsi_debug/adapter*/host*/target*/*/block/* | head -1 | xargs basename`
DISK_DIR=`ls -d /sys/block/$DEVICE/device/scsi_disk/*`
echo "using scsi device $DEVICE"
echo "-1" >/sys/bus/pseudo/drivers/scsi_debug/every_nth
echo "temporary write through" >$DISK_DIR/cache_type
echo "128" >/sys/bus/pseudo/drivers/scsi_debug/opts
echo none > /sys/block/$DEVICE/queue/scheduler
dd if=/dev/$DEVICE of=/dev/null bs=1M iflag=direct count=1 &
sleep 5
echo "0" >/sys/bus/pseudo/drivers/scsi_debug/opts
wait
echo "SUCCESS"
Fixes: 0df21c86bdbf ("scsi: implement .get_budget and .put_budget for blk-mq")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Tested-by: Holger Hoffstätte <holger@applied-asynchrony.com>
Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
drivers/scsi/scsi_lib.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9e242583ab83..175981058396 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1994,6 +1994,8 @@ static bool scsi_mq_get_budget(struct blk_mq_hw_ctx *hctx)
out_put_device:
put_device(&sdev->sdev_gendev);
out:
+ if (atomic_read(&sdev->device_busy) == 0 && !scsi_device_blocked(sdev))
+ blk_mq_delay_run_hw_queue(hctx, SCSI_QUEUE_DELAY);
return false;
}
--
2.7.4
^ permalink raw reply related
* [stable-4.14 20/23] blk-mq: fix issue with shared tag queue re-running
From: Jack Wang @ 2018-07-23 13:24 UTC (permalink / raw)
To: gregkh, stable; +Cc: Jens Axboe, Jack Wang
In-Reply-To: <1532352285-13589-1-git-send-email-jinpuwang@gmail.com>
From: Jens Axboe <axboe@kernel.dk>
commit eb619fdb2d4cb8b3d3419e9113921e87e7daf557 upstream
This patch attempts to make the case of hctx re-running on driver tag
failure more robust. Without this patch, it's pretty easy to trigger a
stall condition with shared tags. An example is using null_blk like
this:
modprobe null_blk queue_mode=2 nr_devices=4 shared_tags=1 submit_queues=1 hw_queue_depth=1
which sets up 4 devices, sharing the same tag set with a depth of 1.
Running a fio job ala:
[global]
bs=4k
rw=randread
norandommap
direct=1
ioengine=libaio
iodepth=4
[nullb0]
filename=/dev/nullb0
[nullb1]
filename=/dev/nullb1
[nullb2]
filename=/dev/nullb2
[nullb3]
filename=/dev/nullb3
will inevitably end with one or more threads being stuck waiting for a
scheduler tag. That IO is then stuck forever, until someone else
triggers a run of the queue.
Ensure that we always re-run the hardware queue, if the driver tag we
were waiting for got freed before we added our leftover request entries
back on the dispatch list.
Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>
Tested-by: Bart Van Assche <bart.vanassche@wdc.com>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Omar Sandoval <osandov@fb.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
block/blk-mq-debugfs.c | 1 -
block/blk-mq.c | 85 ++++++++++++++++++++++++++++----------------------
include/linux/blk-mq.h | 5 ++-
3 files changed, 50 insertions(+), 41 deletions(-)
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index d95439154556..fe880e194cdb 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -180,7 +180,6 @@ static const char *const hctx_state_name[] = {
HCTX_STATE_NAME(STOPPED),
HCTX_STATE_NAME(TAG_ACTIVE),
HCTX_STATE_NAME(SCHED_RESTART),
- HCTX_STATE_NAME(TAG_WAITING),
HCTX_STATE_NAME(START_ON_RUN),
};
#undef HCTX_STATE_NAME
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 657cfde50cda..7b45f20ae7c1 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -984,49 +984,64 @@ bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx,
return rq->tag != -1;
}
-static int blk_mq_dispatch_wake(wait_queue_entry_t *wait, unsigned mode, int flags,
- void *key)
+static int blk_mq_dispatch_wake(wait_queue_entry_t *wait, unsigned mode,
+ int flags, void *key)
{
struct blk_mq_hw_ctx *hctx;
hctx = container_of(wait, struct blk_mq_hw_ctx, dispatch_wait);
- list_del(&wait->entry);
- clear_bit_unlock(BLK_MQ_S_TAG_WAITING, &hctx->state);
+ list_del_init(&wait->entry);
blk_mq_run_hw_queue(hctx, true);
return 1;
}
-static bool blk_mq_dispatch_wait_add(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_dispatch_wait_add(struct blk_mq_hw_ctx **hctx,
+ struct request *rq)
{
+ struct blk_mq_hw_ctx *this_hctx = *hctx;
+ wait_queue_entry_t *wait = &this_hctx->dispatch_wait;
struct sbq_wait_state *ws;
+ if (!list_empty_careful(&wait->entry))
+ return false;
+
+ spin_lock(&this_hctx->lock);
+ if (!list_empty(&wait->entry)) {
+ spin_unlock(&this_hctx->lock);
+ return false;
+ }
+
+ ws = bt_wait_ptr(&this_hctx->tags->bitmap_tags, this_hctx);
+ add_wait_queue(&ws->wait, wait);
+
/*
- * The TAG_WAITING bit serves as a lock protecting hctx->dispatch_wait.
- * The thread which wins the race to grab this bit adds the hardware
- * queue to the wait queue.
+ * It's possible that a tag was freed in the window between the
+ * allocation failure and adding the hardware queue to the wait
+ * queue.
*/
- if (test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state) ||
- test_and_set_bit_lock(BLK_MQ_S_TAG_WAITING, &hctx->state))
+ if (!blk_mq_get_driver_tag(rq, hctx, false)) {
+ spin_unlock(&this_hctx->lock);
return false;
-
- init_waitqueue_func_entry(&hctx->dispatch_wait, blk_mq_dispatch_wake);
- ws = bt_wait_ptr(&hctx->tags->bitmap_tags, hctx);
+ }
/*
- * As soon as this returns, it's no longer safe to fiddle with
- * hctx->dispatch_wait, since a completion can wake up the wait queue
- * and unlock the bit.
+ * We got a tag, remove ourselves from the wait queue to ensure
+ * someone else gets the wakeup.
*/
- add_wait_queue(&ws->wait, &hctx->dispatch_wait);
+ spin_lock_irq(&ws->wait.lock);
+ list_del_init(&wait->entry);
+ spin_unlock_irq(&ws->wait.lock);
+ spin_unlock(&this_hctx->lock);
return true;
}
bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
- bool got_budget)
+ bool got_budget)
{
struct blk_mq_hw_ctx *hctx;
struct request *rq, *nxt;
+ bool no_tag = false;
int errors, queued;
if (list_empty(list))
@@ -1046,22 +1061,15 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
if (!blk_mq_get_driver_tag(rq, &hctx, false)) {
/*
* The initial allocation attempt failed, so we need to
- * rerun the hardware queue when a tag is freed.
+ * rerun the hardware queue when a tag is freed. The
+ * waitqueue takes care of that. If the queue is run
+ * before we add this entry back on the dispatch list,
+ * we'll re-run it below.
*/
- if (!blk_mq_dispatch_wait_add(hctx)) {
- if (got_budget)
- blk_mq_put_dispatch_budget(hctx);
- break;
- }
-
- /*
- * It's possible that a tag was freed in the window
- * between the allocation failure and adding the
- * hardware queue to the wait queue.
- */
- if (!blk_mq_get_driver_tag(rq, &hctx, false)) {
+ if (!blk_mq_dispatch_wait_add(&hctx, rq)) {
if (got_budget)
blk_mq_put_dispatch_budget(hctx);
+ no_tag = true;
break;
}
}
@@ -1126,10 +1134,10 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
* it is no longer set that means that it was cleared by another
* thread and hence that a queue rerun is needed.
*
- * If TAG_WAITING is set that means that an I/O scheduler has
- * been configured and another thread is waiting for a driver
- * tag. To guarantee fairness, do not rerun this hardware queue
- * but let the other thread grab the driver tag.
+ * If 'no_tag' is set, that means that we failed getting
+ * a driver tag with an I/O scheduler attached. If our dispatch
+ * waitqueue is no longer active, ensure that we run the queue
+ * AFTER adding our entries back to the list.
*
* If no I/O scheduler has been configured it is possible that
* the hardware queue got stopped and restarted before requests
@@ -1141,8 +1149,8 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
* returning BLK_STS_RESOURCE. Two exceptions are scsi-mq
* and dm-rq.
*/
- if (!blk_mq_sched_needs_restart(hctx) &&
- !test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state))
+ if (!blk_mq_sched_needs_restart(hctx) ||
+ (no_tag && list_empty_careful(&hctx->dispatch_wait.entry)))
blk_mq_run_hw_queue(hctx, true);
}
@@ -2029,6 +2037,9 @@ static int blk_mq_init_hctx(struct request_queue *q,
hctx->nr_ctx = 0;
+ init_waitqueue_func_entry(&hctx->dispatch_wait, blk_mq_dispatch_wake);
+ INIT_LIST_HEAD(&hctx->dispatch_wait.entry);
+
if (set->ops->init_hctx &&
set->ops->init_hctx(hctx, set->driver_data, hctx_idx))
goto free_bitmap;
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 4564bd216431..cc51f940aec4 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -36,7 +36,7 @@ struct blk_mq_hw_ctx {
struct blk_mq_ctx **ctxs;
unsigned int nr_ctx;
- wait_queue_entry_t dispatch_wait;
+ wait_queue_entry_t dispatch_wait;
atomic_t wait_index;
struct blk_mq_tags *tags;
@@ -182,8 +182,7 @@ enum {
BLK_MQ_S_STOPPED = 0,
BLK_MQ_S_TAG_ACTIVE = 1,
BLK_MQ_S_SCHED_RESTART = 2,
- BLK_MQ_S_TAG_WAITING = 3,
- BLK_MQ_S_START_ON_RUN = 4,
+ BLK_MQ_S_START_ON_RUN = 3,
BLK_MQ_MAX_DEPTH = 10240,
--
2.7.4
^ permalink raw reply related
* [stable-4.14 21/23] blk-mq: only run the hardware queue if IO is pending
From: Jack Wang @ 2018-07-23 13:24 UTC (permalink / raw)
To: gregkh, stable; +Cc: Jens Axboe, Jack Wang
In-Reply-To: <1532352285-13589-1-git-send-email-jinpuwang@gmail.com>
From: Jens Axboe <axboe@kernel.dk>
commit 79f720a751cad613620d0237e3b44f89f4a69181 upstream
Currently we are inconsistent in when we decide to run the queue. Using
blk_mq_run_hw_queues() we check if the hctx has pending IO before
running it, but we don't do that from the individual queue run function,
blk_mq_run_hw_queue(). This results in a lot of extra and pointless
queue runs, potentially, on flush requests and (much worse) on tag
starvation situations. This is observable just looking at top output,
with lots of kworkers active. For the !async runs, it just adds to the
CPU overhead of blk-mq.
Move the has-pending check into the run function instead of having
callers do it.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
block/blk-mq-sched.c | 7 +------
block/blk-mq.c | 18 +++++++++++-------
block/blk-mq.h | 2 --
include/linux/blk-mq.h | 2 +-
4 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 6f4bdb8209f7..c117bd8fd1f6 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -81,12 +81,7 @@ static bool blk_mq_sched_restart_hctx(struct blk_mq_hw_ctx *hctx)
} else
clear_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
- if (blk_mq_hctx_has_pending(hctx)) {
- blk_mq_run_hw_queue(hctx, true);
- return true;
- }
-
- return false;
+ return blk_mq_run_hw_queue(hctx, true);
}
/*
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 7b45f20ae7c1..b3f15d9c7c5a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -60,10 +60,10 @@ static int blk_mq_poll_stats_bkt(const struct request *rq)
/*
* Check if any of the ctx's have pending work in this hardware queue
*/
-bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
{
- return sbitmap_any_bit_set(&hctx->ctx_map) ||
- !list_empty_careful(&hctx->dispatch) ||
+ return !list_empty_careful(&hctx->dispatch) ||
+ sbitmap_any_bit_set(&hctx->ctx_map) ||
blk_mq_sched_has_work(hctx);
}
@@ -1261,9 +1261,14 @@ void blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs)
}
EXPORT_SYMBOL(blk_mq_delay_run_hw_queue);
-void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
+bool blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
{
- __blk_mq_delay_run_hw_queue(hctx, async, 0);
+ if (blk_mq_hctx_has_pending(hctx)) {
+ __blk_mq_delay_run_hw_queue(hctx, async, 0);
+ return true;
+ }
+
+ return false;
}
EXPORT_SYMBOL(blk_mq_run_hw_queue);
@@ -1273,8 +1278,7 @@ void blk_mq_run_hw_queues(struct request_queue *q, bool async)
int i;
queue_for_each_hw_ctx(q, hctx, i) {
- if (!blk_mq_hctx_has_pending(hctx) ||
- blk_mq_hctx_stopped(hctx))
+ if (blk_mq_hctx_stopped(hctx))
continue;
blk_mq_run_hw_queue(hctx, async);
diff --git a/block/blk-mq.h b/block/blk-mq.h
index a5c2b46951e4..1b849a163e9d 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -27,14 +27,12 @@ struct blk_mq_ctx {
struct kobject kobj;
} ____cacheline_aligned_in_smp;
-void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
void blk_mq_freeze_queue(struct request_queue *q);
void blk_mq_free_queue(struct request_queue *q);
int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr);
void blk_mq_wake_waiters(struct request_queue *q);
bool blk_mq_dispatch_rq_list(struct request_queue *, struct list_head *, bool);
void blk_mq_flush_busy_ctxs(struct blk_mq_hw_ctx *hctx, struct list_head *list);
-bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx);
bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx,
bool wait);
struct request *blk_mq_dequeue_from_ctx(struct blk_mq_hw_ctx *hctx,
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index cc51f940aec4..d0162ea97078 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -261,7 +261,7 @@ void blk_mq_start_stopped_hw_queues(struct request_queue *q, bool async);
void blk_mq_quiesce_queue(struct request_queue *q);
void blk_mq_unquiesce_queue(struct request_queue *q);
void blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs);
-void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
+bool blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
void blk_mq_run_hw_queues(struct request_queue *q, bool async);
void blk_mq_delay_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs);
void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset,
--
2.7.4
^ permalink raw reply related
* [stable-4.14 19/23] blk-mq: put driver tag if dispatch budget can't be got
From: Jack Wang @ 2018-07-23 13:24 UTC (permalink / raw)
To: gregkh, stable; +Cc: Ming Lei, Jens Axboe, Jack Wang
In-Reply-To: <1532352285-13589-1-git-send-email-jinpuwang@gmail.com>
From: Ming Lei <ming.lei@redhat.com>
commit 0c6af1ccd5fd9ac640aef01c8de0043837451a04 upstream
We have to put the driver tag if dispatch budget can't be got, otherwise
it might cause IO deadlock, especially in case that size of tags is very
small.
Fixes: de1482974080(blk-mq: introduce .get_budget and .put_budget in blk_mq_ops)
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
block/blk-mq.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index b490a1c2acc8..657cfde50cda 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1066,8 +1066,10 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
}
}
- if (!got_budget && !blk_mq_get_dispatch_budget(hctx))
+ if (!got_budget && !blk_mq_get_dispatch_budget(hctx)) {
+ blk_mq_put_driver_tag(rq);
break;
+ }
list_del_init(&rq->queuelist);
--
2.7.4
^ permalink raw reply related
* [stable-4.14 18/23] Revert "blk-mq: don't handle TAG_SHARED in restart"
From: Jack Wang @ 2018-07-23 13:24 UTC (permalink / raw)
To: gregkh, stable; +Cc: Jens Axboe, Jack Wang
In-Reply-To: <1532352285-13589-1-git-send-email-jinpuwang@gmail.com>
From: Jens Axboe <axboe@kernel.dk>
commit 05b79413946d8b2b58999ea1ae844b6fc3c54f61 upstream
This reverts commit 358a3a6bccb74da9d63a26b2dd5f09f1e9970e0b.
We have cases that aren't covered 100% in the drivers, so for now
we have to retain the shared tag restart loops.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
block/blk-mq-sched.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 74 insertions(+), 4 deletions(-)
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 01a43fed6b8c..6f4bdb8209f7 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -68,17 +68,25 @@ static void blk_mq_sched_mark_restart_hctx(struct blk_mq_hw_ctx *hctx)
set_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
}
-void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_sched_restart_hctx(struct blk_mq_hw_ctx *hctx)
{
if (!test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
- return;
+ return false;
- clear_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
+ if (hctx->flags & BLK_MQ_F_TAG_SHARED) {
+ struct request_queue *q = hctx->queue;
+
+ if (test_and_clear_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
+ atomic_dec(&q->shared_hctx_restart);
+ } else
+ clear_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
if (blk_mq_hctx_has_pending(hctx)) {
blk_mq_run_hw_queue(hctx, true);
- return;
+ return true;
}
+
+ return false;
}
/*
@@ -362,6 +370,68 @@ static bool blk_mq_sched_bypass_insert(struct blk_mq_hw_ctx *hctx,
return false;
}
+/**
+ * list_for_each_entry_rcu_rr - iterate in a round-robin fashion over rcu list
+ * @pos: loop cursor.
+ * @skip: the list element that will not be examined. Iteration starts at
+ * @skip->next.
+ * @head: head of the list to examine. This list must have at least one
+ * element, namely @skip.
+ * @member: name of the list_head structure within typeof(*pos).
+ */
+#define list_for_each_entry_rcu_rr(pos, skip, head, member) \
+ for ((pos) = (skip); \
+ (pos = (pos)->member.next != (head) ? list_entry_rcu( \
+ (pos)->member.next, typeof(*pos), member) : \
+ list_entry_rcu((pos)->member.next->next, typeof(*pos), member)), \
+ (pos) != (skip); )
+
+/*
+ * Called after a driver tag has been freed to check whether a hctx needs to
+ * be restarted. Restarts @hctx if its tag set is not shared. Restarts hardware
+ * queues in a round-robin fashion if the tag set of @hctx is shared with other
+ * hardware queues.
+ */
+void blk_mq_sched_restart(struct blk_mq_hw_ctx *const hctx)
+{
+ struct blk_mq_tags *const tags = hctx->tags;
+ struct blk_mq_tag_set *const set = hctx->queue->tag_set;
+ struct request_queue *const queue = hctx->queue, *q;
+ struct blk_mq_hw_ctx *hctx2;
+ unsigned int i, j;
+
+ if (set->flags & BLK_MQ_F_TAG_SHARED) {
+ /*
+ * If this is 0, then we know that no hardware queues
+ * have RESTART marked. We're done.
+ */
+ if (!atomic_read(&queue->shared_hctx_restart))
+ return;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu_rr(q, queue, &set->tag_list,
+ tag_set_list) {
+ queue_for_each_hw_ctx(q, hctx2, i)
+ if (hctx2->tags == tags &&
+ blk_mq_sched_restart_hctx(hctx2))
+ goto done;
+ }
+ j = hctx->queue_num + 1;
+ for (i = 0; i < queue->nr_hw_queues; i++, j++) {
+ if (j == queue->nr_hw_queues)
+ j = 0;
+ hctx2 = queue->queue_hw_ctx[j];
+ if (hctx2->tags == tags &&
+ blk_mq_sched_restart_hctx(hctx2))
+ break;
+ }
+done:
+ rcu_read_unlock();
+ } else {
+ blk_mq_sched_restart_hctx(hctx);
+ }
+}
+
void blk_mq_sched_insert_request(struct request *rq, bool at_head,
bool run_queue, bool async, bool can_block)
{
--
2.7.4
^ permalink raw reply related
* [stable-4.14 17/23] blk-mq: don't allocate driver tag upfront for flush rq
From: Jack Wang @ 2018-07-23 13:24 UTC (permalink / raw)
To: gregkh, stable; +Cc: Ming Lei, Jens Axboe, Jack Wang
In-Reply-To: <1532352285-13589-1-git-send-email-jinpuwang@gmail.com>
From: Ming Lei <ming.lei@redhat.com>
commit 923218f6166a84688973acdc39094f3bee1e9ad4 upstream
The idea behind it is simple:
1) for none scheduler, driver tag has to be borrowed for flush rq,
otherwise we may run out of tag, and that causes an IO hang. And
get/put driver tag is actually noop for none, so reordering tags
isn't necessary at all.
2) for a real I/O scheduler, we need not allocate a driver tag upfront
for flush rq. It works just fine to follow the same approach as
normal requests: allocate driver tag for each rq just before calling
->queue_rq().
One driver visible change is that the driver tag isn't shared in the
flush request sequence. That won't be a problem, since we always do that
in legacy path.
Then flush rq need not be treated specially wrt. get/put driver tag.
This cleans up the code - for instance, reorder_tags_to_front() can be
removed, and we needn't worry about request ordering in dispatch list
for avoiding I/O deadlock.
Also we have to put the driver tag before requeueing.
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
block/blk-flush.c | 35 ++++++++++++++++++++++++++---------
block/blk-mq-sched.c | 42 +++++-------------------------------------
block/blk-mq.c | 41 ++++++-----------------------------------
3 files changed, 37 insertions(+), 81 deletions(-)
diff --git a/block/blk-flush.c b/block/blk-flush.c
index a9773d2075ac..f17170675917 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -231,8 +231,13 @@ static void flush_end_io(struct request *flush_rq, blk_status_t error)
/* release the tag's ownership to the req cloned from */
spin_lock_irqsave(&fq->mq_flush_lock, flags);
hctx = blk_mq_map_queue(q, flush_rq->mq_ctx->cpu);
- blk_mq_tag_set_rq(hctx, flush_rq->tag, fq->orig_rq);
- flush_rq->tag = -1;
+ if (!q->elevator) {
+ blk_mq_tag_set_rq(hctx, flush_rq->tag, fq->orig_rq);
+ flush_rq->tag = -1;
+ } else {
+ blk_mq_put_driver_tag_hctx(hctx, flush_rq);
+ flush_rq->internal_tag = -1;
+ }
}
running = &fq->flush_queue[fq->flush_running_idx];
@@ -318,19 +323,26 @@ static bool blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq)
blk_rq_init(q, flush_rq);
/*
- * Borrow tag from the first request since they can't
- * be in flight at the same time. And acquire the tag's
- * ownership for flush req.
+ * In case of none scheduler, borrow tag from the first request
+ * since they can't be in flight at the same time. And acquire
+ * the tag's ownership for flush req.
+ *
+ * In case of IO scheduler, flush rq need to borrow scheduler tag
+ * just for cheating put/get driver tag.
*/
if (q->mq_ops) {
struct blk_mq_hw_ctx *hctx;
flush_rq->mq_ctx = first_rq->mq_ctx;
- flush_rq->tag = first_rq->tag;
- fq->orig_rq = first_rq;
- hctx = blk_mq_map_queue(q, first_rq->mq_ctx->cpu);
- blk_mq_tag_set_rq(hctx, first_rq->tag, flush_rq);
+ if (!q->elevator) {
+ fq->orig_rq = first_rq;
+ flush_rq->tag = first_rq->tag;
+ hctx = blk_mq_map_queue(q, first_rq->mq_ctx->cpu);
+ blk_mq_tag_set_rq(hctx, first_rq->tag, flush_rq);
+ } else {
+ flush_rq->internal_tag = first_rq->internal_tag;
+ }
}
flush_rq->cmd_flags = REQ_OP_FLUSH | REQ_PREFLUSH;
@@ -394,6 +406,11 @@ static void mq_flush_data_end_io(struct request *rq, blk_status_t error)
hctx = blk_mq_map_queue(q, ctx->cpu);
+ if (q->elevator) {
+ WARN_ON(rq->tag < 0);
+ blk_mq_put_driver_tag_hctx(hctx, rq);
+ }
+
/*
* After populating an empty queue, kick it to avoid stall. Read
* the comment in flush_end_io().
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index e7094f44afaf..01a43fed6b8c 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -356,29 +356,12 @@ static bool blk_mq_sched_bypass_insert(struct blk_mq_hw_ctx *hctx,
return true;
}
- if (has_sched) {
+ if (has_sched)
rq->rq_flags |= RQF_SORTED;
- WARN_ON(rq->tag != -1);
- }
return false;
}
-/*
- * Add flush/fua to the queue. If we fail getting a driver tag, then
- * punt to the requeue list. Requeue will re-invoke us from a context
- * that's safe to block from.
- */
-static void blk_mq_sched_insert_flush(struct blk_mq_hw_ctx *hctx,
- struct request *rq, bool can_block)
-{
- if (blk_mq_get_driver_tag(rq, &hctx, can_block)) {
- blk_insert_flush(rq);
- blk_mq_run_hw_queue(hctx, true);
- } else
- blk_mq_add_to_requeue_list(rq, false, true);
-}
-
void blk_mq_sched_insert_request(struct request *rq, bool at_head,
bool run_queue, bool async, bool can_block)
{
@@ -389,10 +372,12 @@ void blk_mq_sched_insert_request(struct request *rq, bool at_head,
/* flush rq in flush machinery need to be dispatched directly */
if (!(rq->rq_flags & RQF_FLUSH_SEQ) && op_is_flush(rq->cmd_flags)) {
- blk_mq_sched_insert_flush(hctx, rq, can_block);
- return;
+ blk_insert_flush(rq);
+ goto run;
}
+ WARN_ON(e && (rq->tag != -1));
+
if (blk_mq_sched_bypass_insert(hctx, !!e, rq))
goto run;
@@ -419,23 +404,6 @@ void blk_mq_sched_insert_requests(struct request_queue *q,
struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, ctx->cpu);
struct elevator_queue *e = hctx->queue->elevator;
- if (e) {
- struct request *rq, *next;
-
- /*
- * We bypass requests that already have a driver tag assigned,
- * which should only be flushes. Flushes are only ever inserted
- * as single requests, so we shouldn't ever hit the
- * WARN_ON_ONCE() below (but let's handle it just in case).
- */
- list_for_each_entry_safe(rq, next, list, queuelist) {
- if (WARN_ON_ONCE(rq->tag != -1)) {
- list_del_init(&rq->queuelist);
- blk_mq_sched_bypass_insert(hctx, true, rq);
- }
- }
- }
-
if (e && e->type->ops.mq.insert_requests)
e->type->ops.mq.insert_requests(hctx, list, false);
else
diff --git a/block/blk-mq.c b/block/blk-mq.c
index aff2ada523c3..b490a1c2acc8 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -655,6 +655,8 @@ static void __blk_mq_requeue_request(struct request *rq)
{
struct request_queue *q = rq->q;
+ blk_mq_put_driver_tag(rq);
+
trace_block_rq_requeue(q, rq);
wbt_requeue(q->rq_wb, &rq->issue_stat);
@@ -982,30 +984,6 @@ bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx,
return rq->tag != -1;
}
-/*
- * If we fail getting a driver tag because all the driver tags are already
- * assigned and on the dispatch list, BUT the first entry does not have a
- * tag, then we could deadlock. For that case, move entries with assigned
- * driver tags to the front, leaving the set of tagged requests in the
- * same order, and the untagged set in the same order.
- */
-static bool reorder_tags_to_front(struct list_head *list)
-{
- struct request *rq, *tmp, *first = NULL;
-
- list_for_each_entry_safe_reverse(rq, tmp, list, queuelist) {
- if (rq == first)
- break;
- if (rq->tag != -1) {
- list_move(&rq->queuelist, list);
- if (!first)
- first = rq;
- }
- }
-
- return first != NULL;
-}
-
static int blk_mq_dispatch_wake(wait_queue_entry_t *wait, unsigned mode, int flags,
void *key)
{
@@ -1066,9 +1044,6 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
rq = list_first_entry(list, struct request, queuelist);
if (!blk_mq_get_driver_tag(rq, &hctx, false)) {
- if (!queued && reorder_tags_to_front(list))
- continue;
-
/*
* The initial allocation attempt failed, so we need to
* rerun the hardware queue when a tag is freed.
@@ -1119,7 +1094,6 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
nxt = list_first_entry(list, struct request, queuelist);
blk_mq_put_driver_tag(nxt);
}
- blk_mq_put_driver_tag_hctx(hctx, rq);
list_add(&rq->queuelist, list);
__blk_mq_requeue_request(rq);
break;
@@ -1706,13 +1680,10 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
if (unlikely(is_flush_fua)) {
blk_mq_put_ctx(data.ctx);
blk_mq_bio_to_request(rq, bio);
- if (q->elevator) {
- blk_mq_sched_insert_request(rq, false, true, true,
- true);
- } else {
- blk_insert_flush(rq);
- blk_mq_run_hw_queue(data.hctx, true);
- }
+
+ /* bypass scheduler for flush rq */
+ blk_insert_flush(rq);
+ blk_mq_run_hw_queue(data.hctx, true);
} else if (plug && q->nr_hw_queues == 1) {
struct request *last = NULL;
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.