* Re: [PATCH v5 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver
From: Matthias Kaehlcke @ 2020-02-14 20:11 UTC (permalink / raw)
To: Sandeep Maheswaram
Cc: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, Felipe Balbi, Stephen Boyd, Doug Anderson,
linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
Chandana Kishori Chiluveru
In-Reply-To: <1581668684-4182-3-git-send-email-sanm@codeaurora.org>
Hi Sandeep,
On Fri, Feb 14, 2020 at 01:54:43PM +0530, Sandeep Maheswaram wrote:
> Add interconnect support in dwc3-qcom driver to vote for bus
> bandwidth.
>
> This requires for two different paths - from USB master to
> DDR slave. The other is from APPS master to USB slave.
>
> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 135 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 133 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 261af9e..2ed6c20 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -13,6 +13,7 @@
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/extcon.h>
> +#include <linux/interconnect.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/phy/phy.h>
> @@ -43,6 +44,14 @@
> #define SDM845_QSCRATCH_SIZE 0x400
> #define SDM845_DWC3_CORE_SIZE 0xcd00
>
> +/* Interconnect path bandwidths in MBps */
> +#define USB_MEMORY_AVG_HS_BW MBps_to_icc(240)
> +#define USB_MEMORY_PEAK_HS_BW MBps_to_icc(700)
> +#define USB_MEMORY_AVG_SS_BW MBps_to_icc(1000)
> +#define USB_MEMORY_PEAK_SS_BW MBps_to_icc(2500)
> +#define APPS_USB_AVG_BW 0
> +#define APPS_USB_PEAK_BW MBps_to_icc(40)
> +
> struct dwc3_acpi_pdata {
> u32 qscratch_base_offset;
> u32 qscratch_base_size;
> @@ -76,8 +85,13 @@ struct dwc3_qcom {
> enum usb_dr_mode mode;
> bool is_suspended;
> bool pm_suspended;
> + struct icc_path *usb_ddr_icc_path;
> + struct icc_path *apps_usb_icc_path;
> };
>
> +static int dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom);
> +static int dwc3_qcom_interconnect_disable(struct dwc3_qcom *qcom);
> +
> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> {
> u32 reg;
> @@ -239,7 +253,7 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> {
> u32 val;
> - int i;
> + int i, ret;
>
> if (qcom->is_suspended)
> return 0;
> @@ -251,6 +265,10 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> for (i = qcom->num_clocks - 1; i >= 0; i--)
> clk_disable_unprepare(qcom->clks[i]);
>
> + ret = dwc3_qcom_interconnect_disable(qcom);
> + if (ret)
> + dev_warn(qcom->dev, "failed to disable interconnect %d\n", ret);
> +
This assumes that all QCA systems with a DWC3 have an interconnect
configuration, however after applying this series SDM845 is the only
platform. You need to track somewhere if the controller in question has
an ICC config for not.
> qcom->is_suspended = true;
> dwc3_qcom_enable_interrupts(qcom);
>
> @@ -276,6 +294,10 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> }
> }
>
> + ret = dwc3_qcom_interconnect_enable(qcom);
> + if (ret)
> + dev_warn(qcom->dev, "failed to enable interconnect %d\n", ret);
> +
same as above
> /* Clear existing events from PHY related to L2 in/out */
> dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
> PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
> @@ -285,6 +307,108 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> return 0;
> }
>
> +
> +/**
> + * dwc3_qcom_interconnect_init() - Get interconnect path handles
> + * @qcom: Pointer to the concerned usb core.
> + *
> + */
> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> +{
> + struct device *dev = qcom->dev;
> + int ret;
> +
> + if (!device_is_bound(&qcom->dwc3->dev))
> + return -EPROBE_DEFER;
> +
> + qcom->usb_ddr_icc_path = of_icc_get(dev, "usb-ddr");
> + if (IS_ERR(qcom->usb_ddr_icc_path)) {
> + dev_err(dev, "Error: (%ld) failed getting usb-ddr path\n",
> + PTR_ERR(qcom->usb_ddr_icc_path));
> + return PTR_ERR(qcom->usb_ddr_icc_path);
> + }
This will break all QCA platforms with DWC3, except SDM845. Instead of
failing you could interpret the basence of the 'usb-ddr' patch in the DT
as signal that the controller has no ICC configuration, and continue without
it (i.e. return 0 from here, don't print an error, at most a dev_info() log),
and track somewhere that the controller has no ICC config.
Alternatively you could check above with of_find_property() whether the
controller has an 'interconnects' property at all. If it doesn't exist
return zero, otherwise return an error if any of the paths doesn't exist,
as you do now.
> +
> + qcom->apps_usb_icc_path = of_icc_get(dev, "apps-usb");
> + if (IS_ERR(qcom->apps_usb_icc_path)) {
> + dev_err(dev, "Error: (%ld) failed getting apps-usb path\n",
> + PTR_ERR(qcom->apps_usb_icc_path));
> + return PTR_ERR(qcom->apps_usb_icc_path);
> + }
Failing here is ok, if 'usb-ddr' exists, we expect the rest of the config
to be in place.
> +
> + ret = dwc3_qcom_interconnect_enable(qcom);
> + if (ret) {
> + dev_err(dev, "failed to enable interconnect %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * dwc3_qcom_interconnect_exit() - Release interconnect path handles
> + * @qcom: Pointer to the concerned usb core.
> + *
> + * This function is used to release interconnect path handle.
> + */
> +static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
> +{
> + icc_put(qcom->usb_ddr_icc_path);
> + icc_put(qcom->apps_usb_icc_path);
> +}
> +
> +/* Currently we only use bandwidth level, so just "enable" interconnects */
> +static int dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom)
> +{
> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> + int ret;
> +
> + if (dwc->maximum_speed == USB_SPEED_SUPER) {
> + ret = icc_set_bw(qcom->usb_ddr_icc_path,
> + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
> + } else {
> + ret = icc_set_bw(qcom->usb_ddr_icc_path,
> + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW);
> + }
nit: curly braces are not needed here
> +
> + if (ret)
> + return ret;
> +
> + ret = icc_set_bw(qcom->apps_usb_icc_path,
> + APPS_USB_AVG_BW, APPS_USB_PEAK_BW);
> + if (ret)
> + icc_set_bw(qcom->usb_ddr_icc_path, 0, 0);
> +
> + return ret;
> +}
> +
> +/* To disable an interconnect, we just set its bandwidth to 0 */
> +static int dwc3_qcom_interconnect_disable(struct dwc3_qcom *qcom)
> +{
> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> + int ret;
> +
> + ret = icc_set_bw(qcom->usb_ddr_icc_path, 0, 0);
> + if (ret)
> + return ret;
> +
> + ret = icc_set_bw(qcom->apps_usb_icc_path, 0, 0);
> + if (ret)
> + goto err_reenable_memory_path;
> +
> + return 0;
> +
> + /* Re-enable things in the event of an error */
> +err_reenable_memory_path:
> + if (dwc->maximum_speed == USB_SPEED_SUPER)
> + icc_set_bw(qcom->usb_ddr_icc_path,
> + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
> + else
> + icc_set_bw(qcom->usb_ddr_icc_path,
> + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW);
instead of the above:
dwc3_qcom_interconnect_enable(qcom);
> +
> + return ret;
> +}
> +
> static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
> {
> struct dwc3_qcom *qcom = data;
> @@ -648,6 +772,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> goto depopulate;
> }
>
> + ret = dwc3_qcom_interconnect_init(qcom);
> + if (ret)
> + goto depopulate;
> +
> qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
>
> /* enable vbus override for device mode */
> @@ -657,7 +785,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> /* register extcon to override sw_vbus on Vbus change later */
> ret = dwc3_qcom_register_extcon(qcom);
> if (ret)
> - goto depopulate;
> + goto interconnect_exit;
>
> device_init_wakeup(&pdev->dev, 1);
> qcom->is_suspended = false;
> @@ -667,6 +795,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>
> return 0;
>
> +interconnect_exit:
> + dwc3_qcom_interconnect_exit(qcom);
> depopulate:
> if (np)
> of_platform_depopulate(&pdev->dev);
> @@ -697,6 +827,7 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
> }
> qcom->num_clocks = 0;
>
> + dwc3_qcom_interconnect_exit(qcom);
> reset_control_assert(qcom->resets);
>
> pm_runtime_allow(dev);
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
^ permalink raw reply
* Re: [PATCH v2 02/21] target/arm: Check aa32_pan in take_aarch32_exception(), not aa64_pan
From: Richard Henderson @ 2020-02-14 20:10 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: Eric Auger, Aaron Lindsay, Philippe Mathieu-Daudé
In-Reply-To: <20200214175116.9164-3-peter.maydell@linaro.org>
On 2/14/20 9:50 AM, Peter Maydell wrote:
> In take_aarch32_exception(), we know we are dealing with a CPU that
> has AArch32, so the right isar_feature test is aa32_pan, not aa64_pan.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> target/arm/helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply
* Re: [PATCH] x86/mce/amd: Fix kobject lifetime
From: Borislav Petkov @ 2020-02-14 20:11 UTC (permalink / raw)
To: Greg KH; +Cc: stable, X86 ML, Yazen Ghannam, LKML
In-Reply-To: <20200214151727.GA3959278@kroah.com>
On Fri, Feb 14, 2020 at 07:17:27AM -0800, Greg KH wrote:
> Does not bother me at all, it's fine to see stuff come by that will end
> up in future trees, it's not noise at all. So no need to suppress
> stable@vger if you don't want to.
Ok, but what about your formletter which you send to people explaining
this is not how you should send a patch to stable?
Like this, for example:
https://lkml.kernel.org/r/20200116100925.GA157179@kroah.com
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* [PATCH 7/7] iotests: Test error handling policies with block-commit
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow
In-Reply-To: <20200214200812.28180-1-kwolf@redhat.com>
This tests both read failure (from the top node) and write failure (to
the base node) for on-error=report/stop/ignore.
As block-commit actually starts two different types of block jobs
(mirror.c for committing the active later, commit.c for intermediate
layers), all tests are run for both cases.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/040 | 283 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/040.out | 4 +-
2 files changed, 285 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 2e7ee0e84f..32c82b4ec6 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -430,6 +430,289 @@ class TestReopenOverlay(ImageCommitTestCase):
def test_reopen_overlay(self):
self.run_commit_test(self.img1, self.img0)
+class TestErrorHandling(iotests.QMPTestCase):
+ image_len = 2 * 1024 * 1024
+
+ def setUp(self):
+ iotests.create_image(backing_img, self.image_len)
+ qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, mid_img)
+ qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
+
+ qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x11 0 512k', mid_img)
+ qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x22 0 512k', test_img)
+
+ self.vm = iotests.VM()
+ self.vm.launch()
+
+ self.blkdebug_file = iotests.file_path("blkdebug.conf")
+
+ def tearDown(self):
+ self.vm.shutdown()
+ os.remove(test_img)
+ os.remove(mid_img)
+ os.remove(backing_img)
+
+ def blockdev_add(self, **kwargs):
+ result = self.vm.qmp('blockdev-add', **kwargs)
+ self.assert_qmp(result, 'return', {})
+
+ def add_block_nodes(self, base_debug=None, mid_debug=None, top_debug=None):
+ self.blockdev_add(node_name='base-file', driver='file',
+ filename=backing_img)
+ self.blockdev_add(node_name='mid-file', driver='file',
+ filename=mid_img)
+ self.blockdev_add(node_name='top-file', driver='file',
+ filename=test_img)
+
+ if base_debug:
+ self.blockdev_add(node_name='base-dbg', driver='blkdebug',
+ image='base-file', inject_error=base_debug)
+ if mid_debug:
+ self.blockdev_add(node_name='mid-dbg', driver='blkdebug',
+ image='mid-file', inject_error=mid_debug)
+ if top_debug:
+ self.blockdev_add(node_name='top-dbg', driver='blkdebug',
+ image='top-file', inject_error=top_debug)
+
+ self.blockdev_add(node_name='base-fmt', driver='raw',
+ file=('base-dbg' if base_debug else 'base-file'))
+ self.blockdev_add(node_name='mid-fmt', driver=iotests.imgfmt,
+ file=('mid-dbg' if mid_debug else 'mid-file'),
+ backing='base-fmt')
+ self.blockdev_add(node_name='top-fmt', driver=iotests.imgfmt,
+ file=('top-dbg' if top_debug else 'top-file'),
+ backing='mid-fmt')
+
+ def run_job(self, expected_events, error_pauses_job=False):
+ match_device = {'data': {'device': 'job0'}}
+ events = [
+ ('BLOCK_JOB_COMPLETED', match_device),
+ ('BLOCK_JOB_CANCELLED', match_device),
+ ('BLOCK_JOB_ERROR', match_device),
+ ('BLOCK_JOB_READY', match_device),
+ ]
+
+ completed = False
+ log = []
+ while not completed:
+ ev = self.vm.events_wait(events, timeout=5.0)
+ if ev['event'] == 'BLOCK_JOB_COMPLETED':
+ completed = True
+ elif ev['event'] == 'BLOCK_JOB_ERROR':
+ if error_pauses_job:
+ result = self.vm.qmp('block-job-resume', device='job0')
+ self.assert_qmp(result, 'return', {})
+ elif ev['event'] == 'BLOCK_JOB_READY':
+ result = self.vm.qmp('block-job-complete', device='job0')
+ self.assert_qmp(result, 'return', {})
+ else:
+ self.fail("Unexpected event: %s" % ev)
+ log.append(iotests.filter_qmp_event(ev))
+
+ self.maxDiff = None
+ self.assertEqual(expected_events, log)
+
+ def event_error(self, op, action):
+ return {
+ 'event': 'BLOCK_JOB_ERROR',
+ 'data': {'action': action, 'device': 'job0', 'operation': op},
+ 'timestamp': {'microseconds': 'USECS', 'seconds': 'SECS'}
+ }
+
+ def event_ready(self):
+ return {
+ 'event': 'BLOCK_JOB_READY',
+ 'data': {'device': 'job0',
+ 'len': 524288,
+ 'offset': 524288,
+ 'speed': 0,
+ 'type': 'commit'},
+ 'timestamp': {'microseconds': 'USECS', 'seconds': 'SECS'},
+ }
+
+ def event_completed(self, errmsg=None, active=True):
+ max_len = 524288 if active else self.image_len
+ data = {
+ 'device': 'job0',
+ 'len': max_len,
+ 'offset': 0 if errmsg else max_len,
+ 'speed': 0,
+ 'type': 'commit'
+ }
+ if errmsg:
+ data['error'] = errmsg
+
+ return {
+ 'event': 'BLOCK_JOB_COMPLETED',
+ 'data': data,
+ 'timestamp': {'microseconds': 'USECS', 'seconds': 'SECS'},
+ }
+
+ def blkdebug_event(self, event, is_raw=False):
+ if event:
+ return [{
+ 'event': event,
+ 'sector': 512 if is_raw else 1024,
+ 'once': True,
+ }]
+ return None
+
+ def prepare_and_start_job(self, on_error, active=True,
+ top_event=None, mid_event=None, base_event=None):
+
+ top_debug = self.blkdebug_event(top_event)
+ mid_debug = self.blkdebug_event(mid_event)
+ base_debug = self.blkdebug_event(base_event, True)
+
+ self.add_block_nodes(top_debug=top_debug, mid_debug=mid_debug,
+ base_debug=base_debug)
+
+ result = self.vm.qmp('block-commit', job_id='job0', device='top-fmt',
+ top_node='top-fmt' if active else 'mid-fmt',
+ base_node='mid-fmt' if active else 'base-fmt',
+ on_error=on_error)
+ self.assert_qmp(result, 'return', {})
+
+ def testActiveReadErrorReport(self):
+ self.prepare_and_start_job('report', top_event='read_aio')
+ self.run_job([
+ self.event_error('read', 'report'),
+ self.event_completed('Input/output error')
+ ])
+
+ self.vm.shutdown()
+ self.assertFalse(iotests.compare_images(test_img, mid_img),
+ 'target image matches source after error')
+
+ def testActiveReadErrorStop(self):
+ self.prepare_and_start_job('stop', top_event='read_aio')
+ self.run_job([
+ self.event_error('read', 'stop'),
+ self.event_ready(),
+ self.event_completed()
+ ], error_pauses_job=True)
+
+ self.vm.shutdown()
+ self.assertTrue(iotests.compare_images(test_img, mid_img),
+ 'target image does not match source after commit')
+
+ def testActiveReadErrorIgnore(self):
+ self.prepare_and_start_job('ignore', top_event='read_aio')
+ self.run_job([
+ self.event_error('read', 'ignore'),
+ self.event_ready(),
+ self.event_completed()
+ ])
+
+ # For commit, 'ignore' actually means retry, so this will succeed
+ self.vm.shutdown()
+ self.assertTrue(iotests.compare_images(test_img, mid_img),
+ 'target image does not match source after commit')
+
+ def testActiveWriteErrorReport(self):
+ self.prepare_and_start_job('report', mid_event='write_aio')
+ self.run_job([
+ self.event_error('write', 'report'),
+ self.event_completed('Input/output error')
+ ])
+
+ self.vm.shutdown()
+ self.assertFalse(iotests.compare_images(test_img, mid_img),
+ 'target image matches source after error')
+
+ def testActiveWriteErrorStop(self):
+ self.prepare_and_start_job('stop', mid_event='write_aio')
+ self.run_job([
+ self.event_error('write', 'stop'),
+ self.event_ready(),
+ self.event_completed()
+ ], error_pauses_job=True)
+
+ self.vm.shutdown()
+ self.assertTrue(iotests.compare_images(test_img, mid_img),
+ 'target image does not match source after commit')
+
+ def testActiveWriteErrorIgnore(self):
+ self.prepare_and_start_job('ignore', mid_event='write_aio')
+ self.run_job([
+ self.event_error('write', 'ignore'),
+ self.event_ready(),
+ self.event_completed()
+ ])
+
+ # For commit, 'ignore' actually means retry, so this will succeed
+ self.vm.shutdown()
+ self.assertTrue(iotests.compare_images(test_img, mid_img),
+ 'target image does not match source after commit')
+
+ def testIntermediateReadErrorReport(self):
+ self.prepare_and_start_job('report', active=False, mid_event='read_aio')
+ self.run_job([
+ self.event_error('read', 'report'),
+ self.event_completed('Input/output error', active=False)
+ ])
+
+ self.vm.shutdown()
+ self.assertFalse(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+ 'target image matches source after error')
+
+ def testIntermediateReadErrorStop(self):
+ self.prepare_and_start_job('stop', active=False, mid_event='read_aio')
+ self.run_job([
+ self.event_error('read', 'stop'),
+ self.event_completed(active=False)
+ ], error_pauses_job=True)
+
+ self.vm.shutdown()
+ self.assertTrue(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+ 'target image does not match source after commit')
+
+ def testIntermediateReadErrorIgnore(self):
+ self.prepare_and_start_job('ignore', active=False, mid_event='read_aio')
+ self.run_job([
+ self.event_error('read', 'ignore'),
+ self.event_completed(active=False)
+ ])
+
+ # For commit, 'ignore' actually means retry, so this will succeed
+ self.vm.shutdown()
+ self.assertTrue(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+ 'target image does not match source after commit')
+
+ def testIntermediateWriteErrorReport(self):
+ self.prepare_and_start_job('report', active=False, base_event='write_aio')
+ self.run_job([
+ self.event_error('write', 'report'),
+ self.event_completed('Input/output error', active=False)
+ ])
+
+ self.vm.shutdown()
+ self.assertFalse(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+ 'target image matches source after error')
+
+ def testIntermediateWriteErrorStop(self):
+ self.prepare_and_start_job('stop', active=False, base_event='write_aio')
+ self.run_job([
+ self.event_error('write', 'stop'),
+ self.event_completed(active=False)
+ ], error_pauses_job=True)
+
+ self.vm.shutdown()
+ self.assertTrue(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+ 'target image does not match source after commit')
+
+ def testIntermediateWriteErrorIgnore(self):
+ self.prepare_and_start_job('ignore', active=False, base_event='write_aio')
+ self.run_job([
+ self.event_error('write', 'ignore'),
+ self.event_completed(active=False)
+ ])
+
+ # For commit, 'ignore' actually means retry, so this will succeed
+ self.vm.shutdown()
+ self.assertTrue(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+ 'target image does not match source after commit')
+
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2', 'qed'],
supported_protocols=['file'])
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index 220a5fa82c..6a917130b6 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-...............................................
+...........................................................
----------------------------------------------------------------------
-Ran 47 tests
+Ran 59 tests
OK
--
2.20.1
^ permalink raw reply related
* [PATCH 4/7] commit: Inline commit_populate()
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow
In-Reply-To: <20200214200812.28180-1-kwolf@redhat.com>
commit_populate() is a very short function and only called in a single
place. Its return value doesn't tell us whether an error happened while
reading or writing, which would be necessary for sending the right data
in the BLOCK_JOB_ERROR QMP event.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/commit.c | 28 ++++++----------------------
1 file changed, 6 insertions(+), 22 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 8189f079d2..2992a1012f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -43,27 +43,6 @@ typedef struct CommitBlockJob {
char *backing_file_str;
} CommitBlockJob;
-static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
- int64_t offset, uint64_t bytes,
- void *buf)
-{
- int ret = 0;
-
- assert(bytes < SIZE_MAX);
-
- ret = blk_co_pread(bs, offset, bytes, buf, 0);
- if (ret < 0) {
- return ret;
- }
-
- ret = blk_co_pwrite(base, offset, bytes, buf, 0);
- if (ret < 0) {
- return ret;
- }
-
- return 0;
-}
-
static int commit_prepare(Job *job)
{
CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
@@ -178,7 +157,12 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
copy = (ret == 1);
trace_commit_one_iteration(s, offset, n, ret);
if (copy) {
- ret = commit_populate(s->top, s->base, offset, n, buf);
+ assert(n < SIZE_MAX);
+
+ ret = blk_co_pread(s->top, offset, n, buf, 0);
+ if (ret >= 0) {
+ ret = blk_co_pwrite(s->base, offset, n, buf, 0);
+ }
}
if (ret < 0) {
BlockErrorAction action =
--
2.20.1
^ permalink raw reply related
* [PATCH 2/7] commit: Remove unused bytes_written
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow
In-Reply-To: <20200214200812.28180-1-kwolf@redhat.com>
The bytes_written variable is only ever written to, it serves no
purpose. This has actually been the case since the commit job was first
introduced in commit 747ff602636.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/commit.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 23c90b3b91..cce898a4f3 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -140,7 +140,6 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
int ret = 0;
int64_t n = 0; /* bytes */
void *buf = NULL;
- int bytes_written = 0;
int64_t len, base_len;
ret = len = blk_getlength(s->top);
@@ -180,7 +179,6 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
trace_commit_one_iteration(s, offset, n, ret);
if (copy) {
ret = commit_populate(s->top, s->base, offset, n, buf);
- bytes_written += n;
}
if (ret < 0) {
BlockErrorAction action =
--
2.20.1
^ permalink raw reply related
* [PATCH v2] fs: Un-inline page_mkwrite_check_truncate
From: Andreas Gruenbacher @ 2020-02-14 20:10 UTC (permalink / raw)
To: Alexander Viro, David Sterba, Jeff Layton, Theodore Ts'o,
Chao Yu, Richard Weinberger
Cc: linux-fsdevel, Andreas Gruenbacher, kbuild test robot, Jan Kara
In-Reply-To: <20200213202423.23455-2-agruenba@redhat.com>
Per review comments from Jan and Ted, un-inline page_mkwrite_check_truncate
and move it to mm/filemap.c. This function doesn't seem worth inlining.
v2: Define page_mkwrite_check_truncate outside the CONFIG_MMU guard in
mm/filemap.c to allow block_page_mkwrite to use this helper on
ARCH=m68k.
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Theodore Y. Ts'o <tytso@mit.edu>
---
include/linux/pagemap.h | 28 +---------------------------
mm/filemap.c | 28 ++++++++++++++++++++++++++++
2 files changed, 29 insertions(+), 27 deletions(-)
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index ccb14b6a16b5..6c9c5b88924d 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -636,32 +636,6 @@ static inline unsigned long dir_pages(struct inode *inode)
PAGE_SHIFT;
}
-/**
- * page_mkwrite_check_truncate - check if page was truncated
- * @page: the page to check
- * @inode: the inode to check the page against
- *
- * Returns the number of bytes in the page up to EOF,
- * or -EFAULT if the page was truncated.
- */
-static inline int page_mkwrite_check_truncate(struct page *page,
- struct inode *inode)
-{
- loff_t size = i_size_read(inode);
- pgoff_t index = size >> PAGE_SHIFT;
- int offset = offset_in_page(size);
-
- if (page->mapping != inode->i_mapping)
- return -EFAULT;
-
- /* page is wholly inside EOF */
- if (page->index < index)
- return PAGE_SIZE;
- /* page is wholly past EOF */
- if (page->index > index || !offset)
- return -EFAULT;
- /* page is partially inside EOF */
- return offset;
-}
+int page_mkwrite_check_truncate(struct page *page, struct inode *inode);
#endif /* _LINUX_PAGEMAP_H */
diff --git a/mm/filemap.c b/mm/filemap.c
index 1784478270e1..eac4f7e84823 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2305,6 +2305,34 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
}
EXPORT_SYMBOL(generic_file_read_iter);
+/**
+ * page_mkwrite_check_truncate - check if page was truncated
+ * @page: the page to check
+ * @inode: the inode to check the page against
+ *
+ * Returns the number of bytes in the page up to EOF,
+ * or -EFAULT if the page was truncated.
+ */
+int page_mkwrite_check_truncate(struct page *page, struct inode *inode)
+{
+ loff_t size = i_size_read(inode);
+ pgoff_t index = size >> PAGE_SHIFT;
+ int offset = offset_in_page(size);
+
+ if (page->mapping != inode->i_mapping)
+ return -EFAULT;
+
+ /* page is wholly inside EOF */
+ if (page->index < index)
+ return PAGE_SIZE;
+ /* page is wholly past EOF */
+ if (page->index > index || !offset)
+ return -EFAULT;
+ /* page is partially inside EOF */
+ return offset;
+}
+EXPORT_SYMBOL(page_mkwrite_check_truncate);
+
#ifdef CONFIG_MMU
#define MMAP_LOTSAMISS (100)
/*
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v2 2/4] libnvdimm/namespace: Enforce memremap_compat_align()
From: Aneesh Kumar K.V @ 2020-02-14 16:55 UTC (permalink / raw)
To: Jeff Moyer, Dan Williams
Cc: Vishal L Verma, linuxppc-dev, Linux Kernel Mailing List,
linux-nvdimm
In-Reply-To: <x49h7ztdsp5.fsf@segfault.boston.devel.redhat.com>
On 2/14/20 10:14 PM, Jeff Moyer wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
>
>> On Thu, Feb 13, 2020 at 1:55 PM Jeff Moyer <jmoyer@redhat.com> wrote:
>>>
>>> Dan Williams <dan.j.williams@intel.com> writes:
>>>
>>>> The pmem driver on PowerPC crashes with the following signature when
>>>> instantiating misaligned namespaces that map their capacity via
>>>> memremap_pages().
>>>>
>>>> BUG: Unable to handle kernel data access at 0xc001000406000000
>>>> Faulting instruction address: 0xc000000000090790
>>>> NIP [c000000000090790] arch_add_memory+0xc0/0x130
>>>> LR [c000000000090744] arch_add_memory+0x74/0x130
>>>> Call Trace:
>>>> arch_add_memory+0x74/0x130 (unreliable)
>>>> memremap_pages+0x74c/0xa30
>>>> devm_memremap_pages+0x3c/0xa0
>>>> pmem_attach_disk+0x188/0x770
>>>> nvdimm_bus_probe+0xd8/0x470
>>>>
>>>> With the assumption that only memremap_pages() has alignment
>>>> constraints, enforce memremap_compat_align() for
>>>> pmem_should_map_pages(), nd_pfn, or nd_dax cases.
>>>>
>>>> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> Cc: Jeff Moyer <jmoyer@redhat.com>
>>>> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> Link: https://lore.kernel.org/r/158041477336.3889308.4581652885008605170.stgit@dwillia2-desk3.amr.corp.intel.com
>>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>>> ---
>>>> drivers/nvdimm/namespace_devs.c | 10 ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
>>>> index 032dc61725ff..aff1f32fdb4f 100644
>>>> --- a/drivers/nvdimm/namespace_devs.c
>>>> +++ b/drivers/nvdimm/namespace_devs.c
>>>> @@ -1739,6 +1739,16 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev)
>>>> return ERR_PTR(-ENODEV);
>>>> }
>>>>
>>>> + if (pmem_should_map_pages(dev) || nd_pfn || nd_dax) {
>>>> + struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
>>>> + resource_size_t start = nsio->res.start;
>>>> +
>>>> + if (!IS_ALIGNED(start | size, memremap_compat_align())) {
>>>> + dev_dbg(&ndns->dev, "misaligned, unable to map\n");
>>>> + return ERR_PTR(-EOPNOTSUPP);
>>>> + }
>>>> + }
>>>> +
>>>> if (is_namespace_pmem(&ndns->dev)) {
>>>> struct nd_namespace_pmem *nspm;
>>>>
>>>
>>> Actually, I take back my ack. :) This prevents a previously working
>>> namespace from being successfully probed/setup.
>>
>> Do you have a test case handy? I can see a potential gap with a
>> namespace that used internal padding to fix up the alignment.
>
> # ndctl list -v -n namespace0.0
> [
> {
> "dev":"namespace0.0",
> "mode":"fsdax",
> "map":"dev",
> "size":52846133248,
> "uuid":"b99f6f6a-2909-4189-9bfa-6eeebd95d40e",
> "raw_uuid":"aff43777-015b-493f-bbf9-7c7b0fe33519",
> "sector_size":512,
> "align":4096,
> "blockdev":"pmem0",
> "numa_node":0
> }
> ]
>
> # cat /sys/bus/nd/devices/region0/mappings
> 6
>
> # grep namespace0.0 /proc/iomem
> 1860000000-24e0003fff : namespace0.0
>
>> The goal of this check is to catch cases that are just going to fail
>> devm_memremap_pages(), and the expectation is that it could not have
>> worked before unless it was ported from another platform, or someone
>> flipped the page-size switch on PowerPC.
>
> On x86, creation and probing of the namespace worked fine before this
> patch. What *doesn't* work is creating another fsdax namespace after
> this one. sector mode namespaces can still be created, though:
>
> [
> {
> "dev":"namespace0.1",
> "mode":"sector",
> "size":53270768640,
> "uuid":"67ea2c74-d4b1-4fc9-9c1a-a7d2a6c2a4a7",
> "sector_size":512,
> "blockdev":"pmem0.1s"
> },
>
> # grep namespace0.1 /proc/iomem
> 24e0004000-3160007fff : namespace0.1
>
>>> I thought we were only going to enforce the alignment for a newly
>>> created namespace? This should only check whether the alignment
>>> works for the current platform.
>>
>> The model is a new default 16MB alignment is enforced at creation
>> time, but if you need to support previously created namespaces then
>> you can manually trim that alignment requirement to no less than
>> memremap_compat_align() because that's the point at which
>> devm_memremap_pages() will start failing or crashing.
>
> The problem is that older kernels did not enforce alignment to
> SUBSECTION_SIZE. We shouldn't prevent those namespaces from being
> accessed. The probe itself will not cause the WARN_ON to trigger.
> Creating new namespaces at misaligned addresses could, but you've
> altered the free space allocation such that we won't hit that anymore.
>
> If I drop this patch, the probe will still work, and allocating new
> namespaces will also work:
>
> # ndctl list
> [
> {
> "dev":"namespace0.1",
> "mode":"sector",
> "size":53270768640,
> "uuid":"67ea2c74-d4b1-4fc9-9c1a-a7d2a6c2a4a7",
> "sector_size":512,
> "blockdev":"pmem0.1s"
> },
> {
> "dev":"namespace0.0",
> "mode":"fsdax",
> "map":"dev",
> "size":52846133248,
> "uuid":"b99f6f6a-2909-4189-9bfa-6eeebd95d40e",
> "sector_size":512,
> "align":4096,
> "blockdev":"pmem0"
> }
> ]
> ndctl create-namespace -m fsdax -s 36g -r 0
> {
> "dev":"namespace0.2",
> "mode":"fsdax",
> "map":"dev",
> "size":"35.44 GiB (38.05 GB)",
> "uuid":"7893264c-c7ef-4cbe-95e1-ccf2aff041fb",
> "sector_size":512,
> "align":2097152,
> "blockdev":"pmem0.2"
> }
>
> proc/iomem:
>
> 1860000000-d55fffffff : Persistent Memory
> 1860000000-24e0003fff : namespace0.0
> 24e0004000-3160007fff : namespace0.1
> 3162000000-3a61ffffff : namespace0.2
>
> So, maybe the right thing is to make memremap_compat_align return
> PAGE_SIZE for x86 instead of SUBSECTION_SIZE?
>
I did that as part of
https://lore.kernel.org/linux-nvdimm/20200120140749.69549-2-aneesh.kumar@linux.ibm.com
and applied the subsection details only when creating new namespace
https://lore.kernel.org/linux-nvdimm/20200120140749.69549-4-aneesh.kumar@linux.ibm.com
But I do agree with the approach that in-order to create a compatible
namespace we need enforce max possible align value across all supported
architectures.
On POWER we should still be able to enforce SUBSECTION_SIZE
restrictions. We did put that as document w.r.t. distributions like Suse
https://www.suse.com/support/kb/doc/?id=7024300
-aneesh
^ permalink raw reply
* [PATCH 5/7] commit: Fix is_read for block_job_error_action()
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow
In-Reply-To: <20200214200812.28180-1-kwolf@redhat.com>
block_job_error_action() needs to know if reading from the top node or
writing to the base node failed so that it can set the right 'operation'
in the BLOCK_JOB_ERROR QMP event.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/commit.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/block/commit.c b/block/commit.c
index 2992a1012f..8e672799af 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -143,6 +143,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
for (offset = 0; offset < len; offset += n) {
bool copy;
+ bool error_in_source = true;
/* Note that even when no rate limit is applied we need to yield
* with no pending I/O here so that bdrv_drain_all() returns.
@@ -162,11 +163,15 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
ret = blk_co_pread(s->top, offset, n, buf, 0);
if (ret >= 0) {
ret = blk_co_pwrite(s->base, offset, n, buf, 0);
+ if (ret < 0) {
+ error_in_source = false;
+ }
}
}
if (ret < 0) {
BlockErrorAction action =
- block_job_error_action(&s->common, s->on_error, false, -ret);
+ block_job_error_action(&s->common, s->on_error,
+ error_in_source, -ret);
if (action == BLOCK_ERROR_ACTION_REPORT) {
goto out;
} else {
--
2.20.1
^ permalink raw reply related
* [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow
In-Reply-To: <20200214200812.28180-1-kwolf@redhat.com>
It is not obvious what 'ignore' actually means for block jobs: It could
be continuing the job and returning success in the end despite the error
(no block job does this). It could also mean continuing and returning
failure in the end (this is what stream does). And it can mean retrying
the failed request later (this is what backup, commit and mirror do).
This (somewhat inconsistent) behaviour was introduced and described for
stream and mirror in commit ae586d6158. backup and commit were
introduced later and use the same model as mirror.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ef94a29686..395d205fa8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1164,7 +1164,10 @@
# for jobs, cancel the job
#
# @ignore: ignore the error, only report a QMP event (BLOCK_IO_ERROR
-# or BLOCK_JOB_ERROR)
+# or BLOCK_JOB_ERROR). The backup, mirror and commit block jobs retry
+# the failing request later and may still complete successfully. The
+# stream block job continues to stream and will complete with an
+# error.
#
# @enospc: same as @stop on ENOSPC, same as @report otherwise.
#
--
2.20.1
^ permalink raw reply related
* [PATCH 0/7] commit: Expose on-error option in QMP
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow
All other block jobs already provide a QMP option to control the error
handling policy. It's time to add it to commit, too.
Peter, I guess libvirt wants to expose this?
Nir, as you would probably assume, this is motivated by the recent
finding that VDSM has to preallocate the theoretical maximum for commit
operations because QEMU doesn't provide a way to reliably resize the
base image dynamically. If you ever want to improve that code, this will
enable you to do so from the QEMU side.
Kevin Wolf (7):
qapi: Document meaning of 'ignore' BlockdevOnError for jobs
commit: Remove unused bytes_written
commit: Fix argument order for block_job_error_action()
commit: Inline commit_populate()
commit: Fix is_read for block_job_error_action()
commit: Expose on-error option in QMP
iotests: Test error handling policies with block-commit
qapi/block-core.json | 9 +-
block/commit.c | 37 ++---
blockdev.c | 8 +-
tests/qemu-iotests/040 | 283 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/040.out | 4 +-
5 files changed, 309 insertions(+), 32 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: Unknown Postinstall Scriptlet Failing to Defer Boot
From: Jeremy A. Puhlman @ 2020-02-14 20:07 UTC (permalink / raw)
To: Andrew Boos; +Cc: openembedded-core
In-Reply-To: <CAGgh2e_TzXNQejWMog37ad7kf-ruSiRw-rvCC_NpOF-PjMbUhg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2784 bytes --]
If you look at the log.do_rootfs it will give you more or less the exact
failure(using the message below as a guide), the error message below is
basically
the final catchall that says "at least one of them failed, please go fix
it" kind of message.
On 2/14/2020 10:30 AM, Andrew Boos wrote:
> Thanks Jeremy for the hint about update-alternatives. It turns out
> that lmbench was installing a binary called "stream" that conflicts
> with ImageMagick's own stream. All we had to do was remove lmbench
> from our IMAGE_INSTALL and the problem went away. I'm posting the
> original error for anyone that is having the same issue.
>
> ERROR: imx8-image-3.0b3-r0 do_rootfs: Postinstall scriptlets of
> ['imagemagick'] have failed. If the intention is to defer them to
> first boot,
> then please place them into pkg_postinst_ontarget_${PN} ().
> Deferring to first boot via 'exit 1' is no longer supported.
>
> Andrew
>
>
> On Thu, Feb 13, 2020 at 8:02 PM Jeremy A. Puhlman <jpuhlman@mvista.com
> <mailto:jpuhlman@mvista.com>> wrote:
>
> In these kind of cases, by providing the actual error message,
> people can provide more help.
>
> The recipe inherits update-alternatives and sets a large number of
> alternatives. The alternatives are done in the automatically added
> post install scripts. In all likelihood one of the listed links is
> wrong, or already a symlink or something similar.
> On 2/13/2020 4:49 PM, Andrew Boos wrote:
>> Hi,
>>
>> I am working on a bitbake build and I am running into an issue
>> where a postinst scriptlet is failing via "exit 1" to defer to
>> first boot and I cannot figure out which recipe is causing the
>> issue. Does anybody have any tips on identifying which recipe is
>> actually exiting with a failure? The error log says that it is in
>> imagemagick but the recipe does not have any postinst scripts
>> defined. Also, there are no imagemagick .bbappend files in my
>> layers.
>> (https://github.com/openembedded/meta-openembedded/blob/sumo/meta-oe/recipes-support/imagemagick/imagemagick_7.0.7.bb)
>>
>> Thanks,
>> Andrew
>>
>> <http://www.avg.com/email-signature?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=emailclient>
>> Virus-free. www.avg.com
>> <http://www.avg.com/email-signature?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=emailclient>
>>
>>
>> <#m_-7702559278781328841_DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
>>
>
> --
> Jeremy A. Puhlman
> jpuhlman@mvista.com <mailto:jpuhlman@mvista.com>
>
>
>
> --
> *Andrew Boos* | Avilution, LLC
> (c) 251-895-5509
--
Jeremy A. Puhlman
jpuhlman@mvista.com
[-- Attachment #2: Type: text/html, Size: 6917 bytes --]
^ permalink raw reply
* Re: [PATCH 13/15] drm/amdgpu/display: split dp connector registration (v3)
From: Alex Deucher @ 2020-02-14 20:07 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Alex Deucher, Maling list - DRI developers, amd-gfx list
In-Reply-To: <20200214183514.GZ2363188@phenom.ffwll.local>
On Fri, Feb 14, 2020 at 1:35 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Feb 14, 2020 at 12:39:22PM -0500, Alex Deucher wrote:
> > On Fri, Feb 14, 2020 at 2:39 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Fri, Feb 07, 2020 at 04:17:13PM -0500, Alex Deucher wrote:
> > > > Split into init and register functions to avoid a segfault
> > > > in some configs when the load/unload callbacks are removed.
> > > >
> > > > v2:
> > > > - add back accidently dropped has_aux setting
> > > > - set dev in late_register
> > > >
> > > > v3:
> > > > - fix dp cec ordering
> > >
> > > Why did you move this back out of the late_register callback when going
> > > from v2->v3? In i915 we register the cec stuff from ->late_register, like
> >
> > I got a bunch of complaints from the cec code when I had it switched
> > the other way. They went away when I moved it back. I don't remember
> > the exact messages off hand.
>
> Would be interesting to learn want went wrong, just in case there's a core
> bug here somewhere that prevents drivers from tdtr. But definitely no
> reason to hold off this patch.
I'll repo it next week and send it out for posterity. Thanks for the review.
Alex
> -Daniel
>
> >
> > Alex
> >
> > > anything else userspace visible. Maybe follow-up patch (the idea behind
> > > removing the ->load callback is to close all the driver load races,
> > > instead of only open("/dev/dri/0"), which is protected by
> > > drm_global_mutex). On this:
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >
> > > Cheers, Daniel
> > >
> > > >
> > > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > > > ---
> > > > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 16 ++++++++++++++++
> > > > drivers/gpu/drm/amd/amdgpu/atombios_dp.c | 10 ++--------
> > > > .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 7 ++++++-
> > > > 3 files changed, 24 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> > > > index ec1501e3a63a..f355d9a752d2 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> > > > @@ -1461,6 +1461,20 @@ static enum drm_mode_status amdgpu_connector_dp_mode_valid(struct drm_connector
> > > > return MODE_OK;
> > > > }
> > > >
> > > > +static int
> > > > +amdgpu_connector_late_register(struct drm_connector *connector)
> > > > +{
> > > > + struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
> > > > + int r = 0;
> > > > +
> > > > + if (amdgpu_connector->ddc_bus->has_aux) {
> > > > + amdgpu_connector->ddc_bus->aux.dev = amdgpu_connector->base.kdev;
> > > > + r = drm_dp_aux_register(&amdgpu_connector->ddc_bus->aux);
> > > > + }
> > > > +
> > > > + return r;
> > > > +}
> > > > +
> > > > static const struct drm_connector_helper_funcs amdgpu_connector_dp_helper_funcs = {
> > > > .get_modes = amdgpu_connector_dp_get_modes,
> > > > .mode_valid = amdgpu_connector_dp_mode_valid,
> > > > @@ -1475,6 +1489,7 @@ static const struct drm_connector_funcs amdgpu_connector_dp_funcs = {
> > > > .early_unregister = amdgpu_connector_unregister,
> > > > .destroy = amdgpu_connector_destroy,
> > > > .force = amdgpu_connector_dvi_force,
> > > > + .late_register = amdgpu_connector_late_register,
> > > > };
> > > >
> > > > static const struct drm_connector_funcs amdgpu_connector_edp_funcs = {
> > > > @@ -1485,6 +1500,7 @@ static const struct drm_connector_funcs amdgpu_connector_edp_funcs = {
> > > > .early_unregister = amdgpu_connector_unregister,
> > > > .destroy = amdgpu_connector_destroy,
> > > > .force = amdgpu_connector_dvi_force,
> > > > + .late_register = amdgpu_connector_late_register,
> > > > };
> > > >
> > > > void
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
> > > > index ea702a64f807..9b74cfdba7b8 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
> > > > @@ -186,16 +186,10 @@ amdgpu_atombios_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *m
> > > >
> > > > void amdgpu_atombios_dp_aux_init(struct amdgpu_connector *amdgpu_connector)
> > > > {
> > > > - int ret;
> > > > -
> > > > amdgpu_connector->ddc_bus->rec.hpd = amdgpu_connector->hpd.hpd;
> > > > - amdgpu_connector->ddc_bus->aux.dev = amdgpu_connector->base.kdev;
> > > > amdgpu_connector->ddc_bus->aux.transfer = amdgpu_atombios_dp_aux_transfer;
> > > > - ret = drm_dp_aux_register(&amdgpu_connector->ddc_bus->aux);
> > > > - if (!ret)
> > > > - amdgpu_connector->ddc_bus->has_aux = true;
> > > > -
> > > > - WARN(ret, "drm_dp_aux_register_i2c_bus() failed with error %d\n", ret);
> > > > + drm_dp_aux_init(&amdgpu_connector->ddc_bus->aux);
> > > > + amdgpu_connector->ddc_bus->has_aux = true;
> > > > }
> > > >
> > > > /***** general DP utility functions *****/
> > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > > > index 3959c942c88b..d5b9e72f2649 100644
> > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > > > @@ -155,6 +155,11 @@ amdgpu_dm_mst_connector_late_register(struct drm_connector *connector)
> > > > struct amdgpu_dm_connector *amdgpu_dm_connector =
> > > > to_amdgpu_dm_connector(connector);
> > > > struct drm_dp_mst_port *port = amdgpu_dm_connector->port;
> > > > + int r;
> > > > +
> > > > + r = drm_dp_aux_register(&amdgpu_dm_connector->dm_dp_aux.aux);
> > > > + if (r)
> > > > + return r;
> > > >
> > > > #if defined(CONFIG_DEBUG_FS)
> > > > connector_debugfs_init(amdgpu_dm_connector);
> > > > @@ -484,7 +489,7 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
> > > > aconnector->dm_dp_aux.aux.transfer = dm_dp_aux_transfer;
> > > > aconnector->dm_dp_aux.ddc_service = aconnector->dc_link->ddc;
> > > >
> > > > - drm_dp_aux_register(&aconnector->dm_dp_aux.aux);
> > > > + drm_dp_aux_init(&aconnector->dm_dp_aux.aux);
> > > > drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> > > > &aconnector->base);
> > > >
> > > > --
> > > > 2.24.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 13/15] drm/amdgpu/display: split dp connector registration (v3)
From: Alex Deucher @ 2020-02-14 20:07 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Alex Deucher, Maling list - DRI developers, amd-gfx list
In-Reply-To: <20200214183514.GZ2363188@phenom.ffwll.local>
On Fri, Feb 14, 2020 at 1:35 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Feb 14, 2020 at 12:39:22PM -0500, Alex Deucher wrote:
> > On Fri, Feb 14, 2020 at 2:39 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Fri, Feb 07, 2020 at 04:17:13PM -0500, Alex Deucher wrote:
> > > > Split into init and register functions to avoid a segfault
> > > > in some configs when the load/unload callbacks are removed.
> > > >
> > > > v2:
> > > > - add back accidently dropped has_aux setting
> > > > - set dev in late_register
> > > >
> > > > v3:
> > > > - fix dp cec ordering
> > >
> > > Why did you move this back out of the late_register callback when going
> > > from v2->v3? In i915 we register the cec stuff from ->late_register, like
> >
> > I got a bunch of complaints from the cec code when I had it switched
> > the other way. They went away when I moved it back. I don't remember
> > the exact messages off hand.
>
> Would be interesting to learn want went wrong, just in case there's a core
> bug here somewhere that prevents drivers from tdtr. But definitely no
> reason to hold off this patch.
I'll repo it next week and send it out for posterity. Thanks for the review.
Alex
> -Daniel
>
> >
> > Alex
> >
> > > anything else userspace visible. Maybe follow-up patch (the idea behind
> > > removing the ->load callback is to close all the driver load races,
> > > instead of only open("/dev/dri/0"), which is protected by
> > > drm_global_mutex). On this:
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >
> > > Cheers, Daniel
> > >
> > > >
> > > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > > > ---
> > > > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 16 ++++++++++++++++
> > > > drivers/gpu/drm/amd/amdgpu/atombios_dp.c | 10 ++--------
> > > > .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 7 ++++++-
> > > > 3 files changed, 24 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> > > > index ec1501e3a63a..f355d9a752d2 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> > > > @@ -1461,6 +1461,20 @@ static enum drm_mode_status amdgpu_connector_dp_mode_valid(struct drm_connector
> > > > return MODE_OK;
> > > > }
> > > >
> > > > +static int
> > > > +amdgpu_connector_late_register(struct drm_connector *connector)
> > > > +{
> > > > + struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
> > > > + int r = 0;
> > > > +
> > > > + if (amdgpu_connector->ddc_bus->has_aux) {
> > > > + amdgpu_connector->ddc_bus->aux.dev = amdgpu_connector->base.kdev;
> > > > + r = drm_dp_aux_register(&amdgpu_connector->ddc_bus->aux);
> > > > + }
> > > > +
> > > > + return r;
> > > > +}
> > > > +
> > > > static const struct drm_connector_helper_funcs amdgpu_connector_dp_helper_funcs = {
> > > > .get_modes = amdgpu_connector_dp_get_modes,
> > > > .mode_valid = amdgpu_connector_dp_mode_valid,
> > > > @@ -1475,6 +1489,7 @@ static const struct drm_connector_funcs amdgpu_connector_dp_funcs = {
> > > > .early_unregister = amdgpu_connector_unregister,
> > > > .destroy = amdgpu_connector_destroy,
> > > > .force = amdgpu_connector_dvi_force,
> > > > + .late_register = amdgpu_connector_late_register,
> > > > };
> > > >
> > > > static const struct drm_connector_funcs amdgpu_connector_edp_funcs = {
> > > > @@ -1485,6 +1500,7 @@ static const struct drm_connector_funcs amdgpu_connector_edp_funcs = {
> > > > .early_unregister = amdgpu_connector_unregister,
> > > > .destroy = amdgpu_connector_destroy,
> > > > .force = amdgpu_connector_dvi_force,
> > > > + .late_register = amdgpu_connector_late_register,
> > > > };
> > > >
> > > > void
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
> > > > index ea702a64f807..9b74cfdba7b8 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
> > > > @@ -186,16 +186,10 @@ amdgpu_atombios_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *m
> > > >
> > > > void amdgpu_atombios_dp_aux_init(struct amdgpu_connector *amdgpu_connector)
> > > > {
> > > > - int ret;
> > > > -
> > > > amdgpu_connector->ddc_bus->rec.hpd = amdgpu_connector->hpd.hpd;
> > > > - amdgpu_connector->ddc_bus->aux.dev = amdgpu_connector->base.kdev;
> > > > amdgpu_connector->ddc_bus->aux.transfer = amdgpu_atombios_dp_aux_transfer;
> > > > - ret = drm_dp_aux_register(&amdgpu_connector->ddc_bus->aux);
> > > > - if (!ret)
> > > > - amdgpu_connector->ddc_bus->has_aux = true;
> > > > -
> > > > - WARN(ret, "drm_dp_aux_register_i2c_bus() failed with error %d\n", ret);
> > > > + drm_dp_aux_init(&amdgpu_connector->ddc_bus->aux);
> > > > + amdgpu_connector->ddc_bus->has_aux = true;
> > > > }
> > > >
> > > > /***** general DP utility functions *****/
> > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > > > index 3959c942c88b..d5b9e72f2649 100644
> > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > > > @@ -155,6 +155,11 @@ amdgpu_dm_mst_connector_late_register(struct drm_connector *connector)
> > > > struct amdgpu_dm_connector *amdgpu_dm_connector =
> > > > to_amdgpu_dm_connector(connector);
> > > > struct drm_dp_mst_port *port = amdgpu_dm_connector->port;
> > > > + int r;
> > > > +
> > > > + r = drm_dp_aux_register(&amdgpu_dm_connector->dm_dp_aux.aux);
> > > > + if (r)
> > > > + return r;
> > > >
> > > > #if defined(CONFIG_DEBUG_FS)
> > > > connector_debugfs_init(amdgpu_dm_connector);
> > > > @@ -484,7 +489,7 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
> > > > aconnector->dm_dp_aux.aux.transfer = dm_dp_aux_transfer;
> > > > aconnector->dm_dp_aux.ddc_service = aconnector->dc_link->ddc;
> > > >
> > > > - drm_dp_aux_register(&aconnector->dm_dp_aux.aux);
> > > > + drm_dp_aux_init(&aconnector->dm_dp_aux.aux);
> > > > drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> > > > &aconnector->base);
> > > >
> > > > --
> > > > 2.24.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: FYI nautilus branch is locked
From: Sage Weil @ 2020-02-14 20:06 UTC (permalink / raw)
To: Yuri Weinstein
Cc: dev, Development, Ceph, Abhishek Lekshmanan, Nathan Cutler,
Casey Bodley, Patrick Donnelly, Neha Ojha, Durgin, Josh,
David Zafman, Ramana Venkatesh Raja, Tamilarasi Muthamizhan,
Dillaman, Jason, Sadeh-Weinraub, Yehuda, Lekshmanan, Abhishek,
Ilya Dryomov, Jeff Layton, ceph-qe-team, Andrew Schoen, ceph-qa,
Matt Benjamin
In-Reply-To: <CAMMFjmGOqAoBYmmFOWFHTw9NrGQEwNLeUPmw2+5RE+LzVMsuYw@mail.gmail.com>
Just a note, we need to sort out the mimic->nautilus upgrade failure
before getting too far along here
On Fri, 14 Feb 2020, Yuri Weinstein wrote:
> Sorry correction again - 14.2.8
>
> On Fri, Feb 14, 2020 at 11:30 AM Yuri Weinstein <yweinste@redhat.com> wrote:
> >
> > We are getting ready to test 14.2.9 and nautilus branch is locked for
> > merges until it's done.
> >
> > sah1 - 4d5b84085009968f557baaa4209183f1374773cd
> >
> > Nathan, Abhishek pls confirm.
> >
> > Thank you
> > YuriW
> _______________________________________________
> Dev mailing list -- dev@ceph.io
> To unsubscribe send an email to dev-leave@ceph.io
>
>
^ permalink raw reply
* Re: [Intel-gfx] [PATCH 10/19] drm/i915: Nuke arguments to eb_pin_engine
From: Ruhl, Michael J @ 2020-02-14 20:07 UTC (permalink / raw)
To: Maarten Lankhorst, intel-gfx@lists.freedesktop.org
In-Reply-To: <20200214103055.2117836-11-maarten.lankhorst@linux.intel.com>
>-----Original Message-----
>From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>Maarten Lankhorst
>Sent: Friday, February 14, 2020 5:31 AM
>To: intel-gfx@lists.freedesktop.org
>Subject: [Intel-gfx] [PATCH 10/19] drm/i915: Nuke arguments to
>eb_pin_engine
>
>Those arguments are already set as eb.file and eb.args, so kill off
>the extra arguments. This will allow us to move eb_pin_engine() to
>after we reserved all BO's.
"move eb_pin_engine() to"
I think you mean "too"? (as in "move eb_pin_engine also"
Or do you mean "move_eb_pin_engine to <somewhere else>, after we"?
Other than that,
Acked-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Mike
>
>Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>---
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>index a27aa85985bd..d96e7649314c 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>@@ -2393,11 +2393,10 @@ static void eb_unpin_engine(struct
>i915_execbuffer *eb)
> }
>
> static unsigned int
>-eb_select_legacy_ring(struct i915_execbuffer *eb,
>- struct drm_file *file,
>- struct drm_i915_gem_execbuffer2 *args)
>+eb_select_legacy_ring(struct i915_execbuffer *eb)
> {
> struct drm_i915_private *i915 = eb->i915;
>+ struct drm_i915_gem_execbuffer2 *args = eb->args;
> unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
>
> if (user_ring_id != I915_EXEC_BSD &&
>@@ -2412,7 +2411,7 @@ eb_select_legacy_ring(struct i915_execbuffer *eb,
> unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
>
> if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
>- bsd_idx = gen8_dispatch_bsd_engine(i915, file);
>+ bsd_idx = gen8_dispatch_bsd_engine(i915, eb->file);
> } else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
> bsd_idx <= I915_EXEC_BSD_RING2) {
> bsd_idx >>= I915_EXEC_BSD_SHIFT;
>@@ -2437,18 +2436,16 @@ eb_select_legacy_ring(struct i915_execbuffer
>*eb,
> }
>
> static int
>-eb_pin_engine(struct i915_execbuffer *eb,
>- struct drm_file *file,
>- struct drm_i915_gem_execbuffer2 *args)
>+eb_pin_engine(struct i915_execbuffer *eb)
> {
> struct intel_context *ce;
> unsigned int idx;
> int err;
>
> if (i915_gem_context_user_engines(eb->gem_context))
>- idx = args->flags & I915_EXEC_RING_MASK;
>+ idx = eb->args->flags & I915_EXEC_RING_MASK;
> else
>- idx = eb_select_legacy_ring(eb, file, args);
>+ idx = eb_select_legacy_ring(eb);
>
> ce = i915_gem_context_get_engine(eb->gem_context, idx);
> if (IS_ERR(ce))
>@@ -2681,7 +2678,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> if (unlikely(err))
> goto err_destroy;
>
>- err = eb_pin_engine(&eb, file, args);
>+ err = eb_pin_engine(&eb);
> if (unlikely(err))
> goto err_context;
>
>--
>2.25.0.24.g3f081b084b0
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: cpu bound I/O behaviour in linux 5.4 (possibly others)
From: Marc Lehmann @ 2020-02-14 20:07 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs
In-Reply-To: <CAL3q7H7SPyXB+5G6+XtgfviJdBQQSYD1YyJZPX6rbWxhes-+qw@mail.gmail.com>
On Fri, Feb 14, 2020 at 12:20:46PM +0000, Filipe Manana <fdmanana@gmail.com> wrote:
> So you can't deduce that the free space cache is being used, and
> despite being the default, it was not mentioned by Marc if he's not
> using already the free space tree (-o space_cache=v2).
Today, during an idle period, I stopped the news process, waited 15
minutes for I/O tand CPU to become mostly idle, and did this:
# mount -noremount,clear_cache,space_cache=v1 /localvol5
# mount -noremount,clear_cache,space_cache=v2 /localvol5
Which, to my surprise, didn't make btrfs complain too much:
[1378546.558533] BTRFS info (device dm-17): force clearing of disk cache
[1378546.558536] BTRFS info (device dm-17): enabling disk space caching
[1378546.558537] BTRFS info (device dm-17): disk space caching is enabled
[1378553.868438] BTRFS info (device dm-17): enabling free space tree
[1378553.868440] BTRFS info (device dm-17): using free space tree
I don't know if this was effective in clearing the cache, but it didn't
change the behaviour - as soon as the new process started writing files
again, it was at 100% cpu.
--
The choice of a Deliantra, the free code+content MORPG
-----==- _GNU_ http://www.deliantra.net
----==-- _ generation
---==---(_)__ __ ____ __ Marc Lehmann
--==---/ / _ \/ // /\ \/ / schmorp@schmorp.de
-=====/_/_//_/\_,_/ /_/\_\
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: ti: Allocate dais dynamically for TDM and audio graph card
From: Mark Brown @ 2020-02-14 20:05 UTC (permalink / raw)
To: Tony Lindgren
Cc: alsa-devel, linux-omap, Kuninori Morimoto, Aaro Koskinen,
linux-kernel, Merlijn Wajer, Takashi Iwai, Liam Girdwood,
Peter Ujfalusi, Pavel Machek, Sebastian Reichel, Arthur D .,
Jarkko Nikula
In-Reply-To: <20200214170559.GA64767@atomide.com>
[-- Attachment #1.1: Type: text/plain, Size: 937 bytes --]
On Fri, Feb 14, 2020 at 09:05:59AM -0800, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [200214 12:50]:
> > We really shouldn't need dummy DAIs at all I think, if we do it feels
> > like there's a problem. It's quite possible that there is actually a
> > problem here though...
> It's dummy in the droid4 voice call case as mcbsp is not the clock-master
> and there's nothing to configure for mcbsp.
If the McBSP is doing anything at all it should still be properly
represented with the actual device rather than a dummy otherwise we'll
most likely get confused at some point. If it's not doing anything then
we shouldn't even need the dummy. But perhaps I'm confused about this
particular system, I remember some of the OMAP designs were a bit fun.
> But I guess in some cases mcbsp could be the clock-master and then the
> secondary DAI would have ops.
It'd be a bit of an unusual clock design for a phone but yeah.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply
* Re: [PATCH v3 00/12] Enable per-file/directory DAX operations V3
From: Ira Weiny @ 2020-02-14 20:06 UTC (permalink / raw)
To: Dan Williams
Cc: Darrick J. Wong, Jeff Moyer, Linux Kernel Mailing List,
Alexander Viro, Dave Chinner, Christoph Hellwig,
Theodore Y. Ts'o, Jan Kara, linux-ext4, linux-xfs,
linux-fsdevel
In-Reply-To: <CAPcyv4hkWoC+xCqicH1DWzmU2DcpY0at_A6HaBsrdLbZ6qzWow@mail.gmail.com>
On Thu, Feb 13, 2020 at 04:16:17PM -0800, Dan Williams wrote:
> On Thu, Feb 13, 2020 at 3:29 PM Ira Weiny <ira.weiny@intel.com> wrote:
> >
> > On Thu, Feb 13, 2020 at 11:58:39AM -0800, Darrick J. Wong wrote:
> > > On Thu, Feb 13, 2020 at 11:05:13AM -0800, Ira Weiny wrote:
> > > > On Thu, Feb 13, 2020 at 11:01:57AM -0800, 'Ira Weiny' wrote:
> > > > > On Wed, Feb 12, 2020 at 02:49:48PM -0500, Jeff Moyer wrote:
> > > > > > Ira Weiny <ira.weiny@intel.com> writes:
> > > > > >
> > > >
> > > > [snip]
> > > >
> > > > > > Given that we document the dax mount
> > > > > > option as "the way to get dax," it may be a good idea to allow for a
> > > > > > user to selectively disable dax, even when -o dax is specified. Is that
> > > > > > possible?
> > > > >
> > > > > Not with this patch set. And I'm not sure how that would work. The idea was
> > > > > that -o dax was simply an override for users who were used to having their
> > > > > entire FS be dax. We wanted to depreciate the use of "-o dax" in general. The
> > > > > individual settings are saved so I don't think it makes sense to ignore the -o
> > > > > dax in favor of those settings. Basically that would IMO make the -o dax
> > > > > useless.
> > > >
> > > > Oh and I forgot to mention that setting 'dax' on the root of the FS basically
> > > > provides '-o dax' functionality by default with the ability to "turn it off"
> > > > for files.
> > >
> > > Please don't further confuse FS_XFLAG_DAX and S_DAX.
> >
> > Yes... the above text is wrong WRT statx. But setting the physical
> > XFS_DIFLAG2_DAX flag on the root directory will by default cause all files and
> > directories created there to be XFS_DIFLAG2_DAX and so forth on down the tree
> > unless explicitly changed. This will be the same as mounting with '-o dax' but
> > with the ability to turn off dax for individual files. Which I think is the
> > functionality Jeff is wanting.
>
> To be clear you mean turn off XFS_DIFLAG2_DAX, not mask S_DAX when you
> say "turn off dax", right?
Yes.
[disclaimer: the following assumes the underlying 'device' (superblock)
supports DAX]
... which results in S_DAX == false when the file is opened without the mount
option. The key would be that all directories/files created under a root with
XFS_DIFLAG2_DAX == true would inherit their flag and be XFS_DIFLAG2_DAX == true
all the way down the tree. Any file not wanting DAX would need to set
XFS_DIFLAG2_DAX == false. And setting false could be used on a directory to
allow a user or group to not use dax on files in that sub-tree.
Then without '-o dax' (XFS_MOUNT_DAX == false) all files when opened set S_DAX
equal to XFS_DIFLAG2_DAX value. (Directories, as of V4, never get S_DAX set.)
If '-o dax' (XFS_MOUNT_DAX == true) then S_DAX is set on all files.
[IF the underlying 'device' (superblock) does _not_ support DAX]
... S_DAX is _never_ set but the underlying XFS_DIFLAG2_DAX flags can be
toggled and will be inherited as above. Because S_DAX is never set access to
that file will be restricted to "not dax"...[1]
I could go into that level of detail in the doc if needed? I feel like we need
a more general name for XFS_DIFLAG2_DAX if I do.[2]
>
> The mount option simply forces "S_DAX" on all regular files as long as
> the underlying device (or soon to be superblock for virtiofs) supports
> it. There is no method to mask S_DAX when the filesystem was mounted
> with -o dax. Otherwise we would seem to need yet another physical flag
> to "always disable" dax.
Exactly. I don't think we want to support that. From this thread alone it
seems we have enough complexity and that would be another layer...
;-)
Ira
[1] I'm beginning to think that if I type dax one more time I'm going to go
crazy... :-P
[2] I have patches in the wings to introduce EXT4_DAX_FL as an ext4 on disk bit
which would be equivalent to XFS_DIFLAG2_DAX. If anyone wants a better name
let me know.
^ permalink raw reply
* Re: [PATCH] ASoC: ti: Allocate dais dynamically for TDM and audio graph card
From: Mark Brown @ 2020-02-14 20:05 UTC (permalink / raw)
To: Tony Lindgren
Cc: Peter Ujfalusi, Kuninori Morimoto, Liam Girdwood, Jaroslav Kysela,
Takashi Iwai, alsa-devel, linux-kernel, linux-omap, Aaro Koskinen,
Arthur D ., Jarkko Nikula, Merlijn Wajer, Pavel Machek,
Sebastian Reichel
In-Reply-To: <20200214170559.GA64767@atomide.com>
[-- Attachment #1: Type: text/plain, Size: 937 bytes --]
On Fri, Feb 14, 2020 at 09:05:59AM -0800, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [200214 12:50]:
> > We really shouldn't need dummy DAIs at all I think, if we do it feels
> > like there's a problem. It's quite possible that there is actually a
> > problem here though...
> It's dummy in the droid4 voice call case as mcbsp is not the clock-master
> and there's nothing to configure for mcbsp.
If the McBSP is doing anything at all it should still be properly
represented with the actual device rather than a dummy otherwise we'll
most likely get confused at some point. If it's not doing anything then
we shouldn't even need the dummy. But perhaps I'm confused about this
particular system, I remember some of the OMAP designs were a bit fun.
> But I guess in some cases mcbsp could be the clock-master and then the
> secondary DAI would have ops.
It'd be a bit of an unusual clock design for a phone but yeah.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [Letux-kernel] [PATCH v2] net: davicom: dm9000: allow to pass MAC address through mac_addr module parameter
From: H. Nikolaus Schaller @ 2020-02-14 20:05 UTC (permalink / raw)
To: Paul Cercueil
Cc: Andrew Lunn, netdev, Linux Kernel Mailing List, Richard Fontana,
Thomas Gleixner, kernel, Petr Štetiar, David S. Miller,
Heiner Kallweit, Discussions about the Letux Kernel
In-Reply-To: <A686A3C7-09A4-4654-A265-2BDBEF41A7C4@goldelico.com>
> Am 14.02.2020 um 20:38 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>
>
>> Am 14.02.2020 um 20:24 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>
>>
>>> Am 14.02.2020 um 19:47 schrieb Paul Cercueil <paul@crapouillou.net>:
>>>
>>> Hi Nikolaus,
>>>
>>> What I'd suggest is to write a NVMEM driver for the efuse and retrieve the MAC address cleanly with nvmem_get_mac_address().
>>>
>>> It shouldn't be hard to do (there's already code for it in the non-upstream 3.18 kernel for the CI20) and you remove the dependency on uboot.
>>
>> Interesting approach. I have found this:
>>
>> https://lore.kernel.org/patchwork/patch/868158/
>>
>> but it looks as if it was never finished (I could not locate a V3 or anything mainline?)
>> and and it tries to solve other problems as well.
>>
>> And it looks to be much more complex than my "solution" to the immediate problem.
>>
>> I have to study it to know if I can write a nvmem_get_mac_address().
>
> Another question is how to link this very jz4780 specific code to the generic davicom dm9000 driver?
> And where should the new code live. In some jz4780 specific file or elsewhere?
Ok, got it.
nvmem_get_mac_address() is looking for a nvmem cell "mac-address".
So some jz4780 specific driver must provide such cells.
There aren't many examples but it appears as if arch/arm/mach-davinci/board-da830-evm.c
defines and registers nvmem cells.
But maybe it is not difficult to teach the 2018 driver to provide such cells.
BR and thanks,
Nikolaus
>
>>
>> BR,
>> Nikolaus
>>
>>>
>>> -Paul
>>>
>>>
>>> Le ven., févr. 14, 2020 at 17:07, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>>> The MIPS Ingenic CI20 board is shipped with a quite old u-boot
>>>> (ci20-v2013.10 see https://elinux.org/CI20_Dev_Zone). This passes
>>>> the MAC address through dm9000.mac_addr=xx:xx:xx:xx:xx:xx
>>>> kernel module parameter to give the board a fixed MAC address.
>>>> This is not processed by the dm9000 driver which assigns a random
>>>> MAC address on each boot, making DHCP assign a new IP address
>>>> each time.
>>>> So we add a check for the mac_addr module parameter as a last
>>>> resort before assigning a random one. This mechanism can also
>>>> be used outside of u-boot to provide a value through modprobe
>>>> config.
>>>> To parse the MAC address in a new function get_mac_addr() we
>>>> use an copy adapted from the ksz884x.c driver which provides
>>>> the same functionality.
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> drivers/net/ethernet/davicom/dm9000.c | 42 +++++++++++++++++++++++++++
>>>> 1 file changed, 42 insertions(+)
>>>> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
>>>> index 1ea3372775e6..7402030b0352 100644
>>>> --- a/drivers/net/ethernet/davicom/dm9000.c
>>>> +++ b/drivers/net/ethernet/davicom/dm9000.c
>>>> @@ -1409,6 +1409,43 @@ static struct dm9000_plat_data *dm9000_parse_dt(struct device *dev)
>>>> return pdata;
>>>> }
>>>> +static char *mac_addr = ":";
>>>> +module_param(mac_addr, charp, 0);
>>>> +MODULE_PARM_DESC(mac_addr, "MAC address");
>>>> +
>>>> +static void get_mac_addr(struct net_device *ndev, char *macaddr)
>>>> +{
>>>> + int i = 0;
>>>> + int j = 0;
>>>> + int got_num = 0;
>>>> + int num = 0;
>>>> +
>>>> + while (j < ETH_ALEN) {
>>>> + if (macaddr[i]) {
>>>> + int digit;
>>>> +
>>>> + got_num = 1;
>>>> + digit = hex_to_bin(macaddr[i]);
>>>> + if (digit >= 0)
>>>> + num = num * 16 + digit;
>>>> + else if (':' == macaddr[i])
>>>> + got_num = 2;
>>>> + else
>>>> + break;
>>>> + } else if (got_num) {
>>>> + got_num = 2;
>>>> + } else {
>>>> + break;
>>>> + }
>>>> + if (got_num == 2) {
>>>> + ndev->dev_addr[j++] = (u8)num;
>>>> + num = 0;
>>>> + got_num = 0;
>>>> + }
>>>> + i++;
>>>> + }
>>>> +}
>>>> +
>>>> /*
>>>> * Search DM9000 board, allocate space and register it
>>>> */
>>>> @@ -1679,6 +1716,11 @@ dm9000_probe(struct platform_device *pdev)
>>>> ndev->dev_addr[i] = ior(db, i+DM9000_PAR);
>>>> }
>>>> + if (!is_valid_ether_addr(ndev->dev_addr)) {
>>>> + mac_src = "param";
>>>> + get_mac_addr(ndev, mac_addr);
>>>> + }
>>>> +
>>>> if (!is_valid_ether_addr(ndev->dev_addr)) {
>>>> inv_mac_addr = true;
>>>> eth_hw_addr_random(ndev);
>>>> --
>>>> 2.23.0
>>>
>>>
>>
>> _______________________________________________
>> http://projects.goldelico.com/p/gta04-kernel/
>> Letux-kernel mailing list
>> Letux-kernel@openphoenux.org
>> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel
>
> _______________________________________________
> http://projects.goldelico.com/p/gta04-kernel/
> Letux-kernel mailing list
> Letux-kernel@openphoenux.org
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel
^ permalink raw reply
* Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating
From: David Hildenbrand @ 2020-02-14 20:04 UTC (permalink / raw)
To: Peter Xu
Cc: Eduardo Habkost, Juan Quintela, Michael S. Tsirkin,
Richard Henderson, Dr. David Alan Gilbert,
Shameerali Kolothum Thodi, qemu-devel, Shannon Zhao,
Paolo Bonzini, Igor Mammedov, David Hildenbrand, Alex Bennée
In-Reply-To: <20200214194459.GB1193332@xz-x1>
> Am 14.02.2020 um 20:45 schrieb Peter Xu <peterx@redhat.com>:
>
> On Fri, Feb 14, 2020 at 07:26:59PM +0100, David Hildenbrand wrote:
>>>>>> + if (!postcopy_is_running()) {
>>>>>> + Error *err = NULL;
>>>>>> +
>>>>>> + /*
>>>>>> + * Precopy code cannot deal with the size of ram blocks changing at
>>>>>> + * random points in time. We're still running on the source, abort
>>>>>> + * the migration and continue running here. Make sure to wait until
>>>>>> + * migration was canceled.
>>>>>> + */
>>>>>> + error_setg(&err, "RAM resized during precopy.");
>>>>>> + migrate_set_error(migrate_get_current(), err);
>>>>>> + error_free(err);
>>>>>> + migration_cancel();
>>>>>> + } else {
>>>>>> + /*
>>>>>> + * Postcopy code cannot deal with the size of ram blocks changing at
>>>>>> + * random points in time. We're running on the target. Fail hard.
>>>>>> + *
>>>>>> + * TODO: How to handle this in a better way?
>>>>>> + */
>>>>>> + error_report("RAM resized during postcopy.");
>>>>>> + exit(-1);
>>>>>
>>>>> Now I'm rethinking the postcopy case....
>>>>>
>>>>> ram_dirty_bitmap_reload() should only happen during the postcopy
>>>>> recovery, and when that happens the VM should be stopped on both
>>>>> sides. Which means, ram resizing should not trigger there...
>>>>
>>>> But that guest got the chance to run for a bit and eventually reboot
>>>> AFAIK. Also, there are other data races possible when used_length
>>>> suddenly changes, this is just the most obvious one where things will;
>>>> get screwed up.
>>>
>>> Right, the major one could be in ram_load_postcopy() where we call
>>> host_from_ram_block_offset(). However if FW extension is the major
>>> use case then it seems to still work (still better than crashing,
>>> isn't it? :).
>>
>> "Let's close our eyes and hope it will never happen" ? :) No, I don't
>> like that. This screams for a better solution long term, and until then
>> a proper fencing IMHO. We're making here wild guesses about data races
>> and why they might not be that bad in certain cases (did I mention
>> load/store tearing? used_length is not an atomic value ...).
>
> Yeah fencing is good, but crashing a VM while it wasn't going to crash
> is another thing, imho. You can dump an error message if you really
> like, but instead of exit() I really prefer we either still let the
> old way to at least work or really fix it.
I‘ll do whatever Juan/Dave think is best. I am not convinced that there is no way to corrupt data or crash later when the guest is already running again post-reboot and doing real work.
>
> When I say "really fix it", I mean we can even start to think about
> the shrinking case and how to support that for postcopy. For example,
> in the above case host_from_ram_block_offset() will return NULL then,
> and the fix could be that we drop that extra page because we don't
> need that any more, instead of bailing out.
>
I have patches on the list that will make everything exceed used_length inaccessible. If there is still an access, we will crash. Printing a warning might help figure out what went wrong.
I have a patch lying around that allocates the bitmaps only for the used_length. Access outside of that (esp. receiced bitmap) will, well, depends, crash or mess up data. Printing an error might help to figure out what went wrong. Maybe.
Just FYI how I found this issue and why I want to sanitize the code. And we are trying to keep something alive here that never could have worked 100% reliably as it is inherently racy.
Cheers!
^ permalink raw reply
* [PATCH] btrfs: bail out of uuid tree scanning if we're closing
From: Josef Bacik @ 2020-02-14 20:05 UTC (permalink / raw)
To: linux-btrfs, kernel-team
In doing my fsstress+EIO stress testing I started running into issues
where umount would get stuck forever because the uuid checker was
chewing through the thousands of subvolumes I had created. We shouldn't
block umount on this, simply bail if we're unmounting the fs. We need
to make sure we don't mark the UUID tree as ok, so we only set that bit
if we made it through the whole rescan operation, but otherwise this is
completely safe.
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
fs/btrfs/uuid-tree.c | 4 ++++
fs/btrfs/volumes.c | 11 +++++++++--
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/uuid-tree.c b/fs/btrfs/uuid-tree.c
index 76b84f2397b1..30078b74220e 100644
--- a/fs/btrfs/uuid-tree.c
+++ b/fs/btrfs/uuid-tree.c
@@ -278,6 +278,10 @@ int btrfs_uuid_tree_iterate(struct btrfs_fs_info *fs_info,
}
while (1) {
+ if (btrfs_fs_closing(fs_info)) {
+ ret = -EINTR;
+ goto out;
+ }
cond_resched();
leaf = path->nodes[0];
slot = path->slots[0];
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 387f80656476..e40adf38ae5c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4286,6 +4286,7 @@ static int btrfs_uuid_scan_kthread(void *data)
struct btrfs_root_item root_item;
u32 item_size;
struct btrfs_trans_handle *trans = NULL;
+ bool closing = false;
path = btrfs_alloc_path();
if (!path) {
@@ -4298,6 +4299,10 @@ static int btrfs_uuid_scan_kthread(void *data)
key.offset = 0;
while (1) {
+ if (btrfs_fs_closing(fs_info)) {
+ closing = true;
+ break;
+ }
ret = btrfs_search_forward(root, &key, path,
BTRFS_OLDEST_GENERATION);
if (ret) {
@@ -4397,7 +4402,7 @@ static int btrfs_uuid_scan_kthread(void *data)
btrfs_end_transaction(trans);
if (ret)
btrfs_warn(fs_info, "btrfs_uuid_scan_kthread failed %d", ret);
- else
+ else if (!closing)
set_bit(BTRFS_FS_UPDATE_UUID_TREE_GEN, &fs_info->flags);
up(&fs_info->uuid_tree_rescan_sem);
return 0;
@@ -4460,7 +4465,9 @@ static int btrfs_uuid_rescan_kthread(void *data)
*/
ret = btrfs_uuid_tree_iterate(fs_info, btrfs_check_uuid_tree_entry);
if (ret < 0) {
- btrfs_warn(fs_info, "iterating uuid_tree failed %d", ret);
+ if (ret != -EINTR)
+ btrfs_warn(fs_info, "iterating uuid_tree failed %d",
+ ret);
up(&fs_info->uuid_tree_rescan_sem);
return ret;
}
--
2.24.1
^ permalink raw reply related
* Re: [alsa-devel] [RFC PATCH 0/2] soundwire: add master_device/driver support
From: Greg KH @ 2020-02-14 16:49 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, tiwai, linux-kernel, Ranjani Sridharan, vkoul,
broonie, srinivas.kandagatla, jank, slawomir.blauciak, Bard liao,
Rander Wang
In-Reply-To: <20200201042011.5781-1-pierre-louis.bossart@linux.intel.com>
On Fri, Jan 31, 2020 at 10:20:09PM -0600, Pierre-Louis Bossart wrote:
> The SoundWire master representation needs to evolve to take into account:
>
> a) a May 2019 recommendation from Greg KH to remove platform devices
>
> b) the need on Intel platforms to support hardware startup only once
> the power rail dependencies are settled. The SoundWire master is not
> an independent IP at all.
>
> c) the need to deal with external wakes handled by the PCI subsystem
> in specific low-power modes
>
> In case it wasn't clear, the SoundWire subsystem is currently unusable
> with v5.5 on devices hitting the shelves very soon (race conditions,
> power management, suspend/resume, etc). v5.6 will only provide
> interface changes and no functional improvement. We've circled on the
> same concepts since September 2019, and I hope this direction is now
> aligned with the suggestions from Vinod Koul and that we can target
> v5.7 as the 'SoundWire just works(tm)' version.
>
> This series is provided as an RFC since it depends on patches already
> for review.
Both of these look sane to me, nice work!
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply
* Re: [PATCH v2 2/4] libnvdimm/namespace: Enforce memremap_compat_align()
From: Jeff Moyer @ 2020-02-14 16:44 UTC (permalink / raw)
To: Dan Williams
Cc: Aneesh Kumar K.V, Vishal L Verma, linuxppc-dev,
Linux Kernel Mailing List, linux-nvdimm
In-Reply-To: <CAPcyv4hQouRNBcJ4uZ2mysr_aKstLhvUf66gRQ_3QoQNyOy72g@mail.gmail.com>
Dan Williams <dan.j.williams@intel.com> writes:
> On Thu, Feb 13, 2020 at 1:55 PM Jeff Moyer <jmoyer@redhat.com> wrote:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>> > The pmem driver on PowerPC crashes with the following signature when
>> > instantiating misaligned namespaces that map their capacity via
>> > memremap_pages().
>> >
>> > BUG: Unable to handle kernel data access at 0xc001000406000000
>> > Faulting instruction address: 0xc000000000090790
>> > NIP [c000000000090790] arch_add_memory+0xc0/0x130
>> > LR [c000000000090744] arch_add_memory+0x74/0x130
>> > Call Trace:
>> > arch_add_memory+0x74/0x130 (unreliable)
>> > memremap_pages+0x74c/0xa30
>> > devm_memremap_pages+0x3c/0xa0
>> > pmem_attach_disk+0x188/0x770
>> > nvdimm_bus_probe+0xd8/0x470
>> >
>> > With the assumption that only memremap_pages() has alignment
>> > constraints, enforce memremap_compat_align() for
>> > pmem_should_map_pages(), nd_pfn, or nd_dax cases.
>> >
>> > Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> > Cc: Jeff Moyer <jmoyer@redhat.com>
>> > Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> > Link: https://lore.kernel.org/r/158041477336.3889308.4581652885008605170.stgit@dwillia2-desk3.amr.corp.intel.com
>> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> > ---
>> > drivers/nvdimm/namespace_devs.c | 10 ++++++++++
>> > 1 file changed, 10 insertions(+)
>> >
>> > diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
>> > index 032dc61725ff..aff1f32fdb4f 100644
>> > --- a/drivers/nvdimm/namespace_devs.c
>> > +++ b/drivers/nvdimm/namespace_devs.c
>> > @@ -1739,6 +1739,16 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev)
>> > return ERR_PTR(-ENODEV);
>> > }
>> >
>> > + if (pmem_should_map_pages(dev) || nd_pfn || nd_dax) {
>> > + struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
>> > + resource_size_t start = nsio->res.start;
>> > +
>> > + if (!IS_ALIGNED(start | size, memremap_compat_align())) {
>> > + dev_dbg(&ndns->dev, "misaligned, unable to map\n");
>> > + return ERR_PTR(-EOPNOTSUPP);
>> > + }
>> > + }
>> > +
>> > if (is_namespace_pmem(&ndns->dev)) {
>> > struct nd_namespace_pmem *nspm;
>> >
>>
>> Actually, I take back my ack. :) This prevents a previously working
>> namespace from being successfully probed/setup.
>
> Do you have a test case handy? I can see a potential gap with a
> namespace that used internal padding to fix up the alignment.
# ndctl list -v -n namespace0.0
[
{
"dev":"namespace0.0",
"mode":"fsdax",
"map":"dev",
"size":52846133248,
"uuid":"b99f6f6a-2909-4189-9bfa-6eeebd95d40e",
"raw_uuid":"aff43777-015b-493f-bbf9-7c7b0fe33519",
"sector_size":512,
"align":4096,
"blockdev":"pmem0",
"numa_node":0
}
]
# cat /sys/bus/nd/devices/region0/mappings
6
# grep namespace0.0 /proc/iomem
1860000000-24e0003fff : namespace0.0
> The goal of this check is to catch cases that are just going to fail
> devm_memremap_pages(), and the expectation is that it could not have
> worked before unless it was ported from another platform, or someone
> flipped the page-size switch on PowerPC.
On x86, creation and probing of the namespace worked fine before this
patch. What *doesn't* work is creating another fsdax namespace after
this one. sector mode namespaces can still be created, though:
[
{
"dev":"namespace0.1",
"mode":"sector",
"size":53270768640,
"uuid":"67ea2c74-d4b1-4fc9-9c1a-a7d2a6c2a4a7",
"sector_size":512,
"blockdev":"pmem0.1s"
},
# grep namespace0.1 /proc/iomem
24e0004000-3160007fff : namespace0.1
>> I thought we were only going to enforce the alignment for a newly
>> created namespace? This should only check whether the alignment
>> works for the current platform.
>
> The model is a new default 16MB alignment is enforced at creation
> time, but if you need to support previously created namespaces then
> you can manually trim that alignment requirement to no less than
> memremap_compat_align() because that's the point at which
> devm_memremap_pages() will start failing or crashing.
The problem is that older kernels did not enforce alignment to
SUBSECTION_SIZE. We shouldn't prevent those namespaces from being
accessed. The probe itself will not cause the WARN_ON to trigger.
Creating new namespaces at misaligned addresses could, but you've
altered the free space allocation such that we won't hit that anymore.
If I drop this patch, the probe will still work, and allocating new
namespaces will also work:
# ndctl list
[
{
"dev":"namespace0.1",
"mode":"sector",
"size":53270768640,
"uuid":"67ea2c74-d4b1-4fc9-9c1a-a7d2a6c2a4a7",
"sector_size":512,
"blockdev":"pmem0.1s"
},
{
"dev":"namespace0.0",
"mode":"fsdax",
"map":"dev",
"size":52846133248,
"uuid":"b99f6f6a-2909-4189-9bfa-6eeebd95d40e",
"sector_size":512,
"align":4096,
"blockdev":"pmem0"
}
]
ndctl create-namespace -m fsdax -s 36g -r 0
{
"dev":"namespace0.2",
"mode":"fsdax",
"map":"dev",
"size":"35.44 GiB (38.05 GB)",
"uuid":"7893264c-c7ef-4cbe-95e1-ccf2aff041fb",
"sector_size":512,
"align":2097152,
"blockdev":"pmem0.2"
}
proc/iomem:
1860000000-d55fffffff : Persistent Memory
1860000000-24e0003fff : namespace0.0
24e0004000-3160007fff : namespace0.1
3162000000-3a61ffffff : namespace0.2
So, maybe the right thing is to make memremap_compat_align return
PAGE_SIZE for x86 instead of SUBSECTION_SIZE?
-Jeff
^ permalink raw reply
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.