Re: [PATCH v2] iio: trigger: free trigger resource correctly
On 20/01/17 03:47, Alison Schofield wrote:
> These stand-alone trigger drivers were using iio_trigger_put()
> where they should have been using iio_trigger_free(). The
> iio_trigger_put() adds a module_put which is bad since they
> never did a module_get.
>
> In the sysfs driver, module_get/put's are used as triggers are
> added & removed. This extra module_put() occurs on an error path
> in the probe routine (probably rare).
>
> In the bfin-timer & interrupt trigger drivers, the module resources
> are not explicitly managed, so it's doing a put on something that
> was never get'd. It occurs on the probe error path and on the
> remove path (not so rare).
>
> Tested with the sysfs trigger driver.
> The bfin & interrupt drivers were build tested & inspected only.
>
> Signed-off-by: Alison Schofield
This is certainly more consistent.
Lars, could you sanity check as well given the bfin timer is
in here.
Thanks,
Jonathan
> ---
> Changes in v2:
> Renamed probe jump label to say free instead of put.
> Added further explanation to commit log.
>
> Note from v1:
> Patches to use devm_* funcs are ready to follow this for
> the interrupt & bfin-timer triggers.
>
>
> drivers/iio/trigger/iio-trig-interrupt.c | 8
> drivers/iio/trigger/iio-trig-sysfs.c | 2 +-
> drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 4 ++--
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/trigger/iio-trig-interrupt.c
> b/drivers/iio/trigger/iio-trig-interrupt.c
> index 572bc6f..e18f12b 100644
> --- a/drivers/iio/trigger/iio-trig-interrupt.c
> +++ b/drivers/iio/trigger/iio-trig-interrupt.c
> @@ -58,7 +58,7 @@ static int iio_interrupt_trigger_probe(struct
> platform_device *pdev)
> trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
> if (!trig_info) {
> ret = -ENOMEM;
> - goto error_put_trigger;
> + goto error_free_trigger;
> }
> iio_trigger_set_drvdata(trig, trig_info);
> trig_info->irq = irq;
> @@ -83,8 +83,8 @@ static int iio_interrupt_trigger_probe(struct
> platform_device *pdev)
> free_irq(irq, trig);
> error_free_trig_info:
> kfree(trig_info);
> -error_put_trigger:
> - iio_trigger_put(trig);
> +error_free_trigger:
> + iio_trigger_free(trig);
> error_ret:
> return ret;
> }
> @@ -99,7 +99,7 @@ static int iio_interrupt_trigger_remove(struct
> platform_device *pdev)
> iio_trigger_unregister(trig);
> free_irq(trig_info->irq, trig);
> kfree(trig_info);
> - iio_trigger_put(trig);
> + iio_trigger_free(trig);
>
> return 0;
> }
> diff --git a/drivers/iio/trigger/iio-trig-sysfs.c
> b/drivers/iio/trigger/iio-trig-sysfs.c
> index 3dfab2b..202e8b8 100644
> --- a/drivers/iio/trigger/iio-trig-sysfs.c
> +++ b/drivers/iio/trigger/iio-trig-sysfs.c
> @@ -174,7 +174,7 @@ static int iio_sysfs_trigger_probe(int id)
> return 0;
>
> out2:
> - iio_trigger_put(t->trig);
> + iio_trigger_free(t->trig);
> free_t:
> kfree(t);
> out1:
> diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> index 9658f20..4e0b4ee 100644
> --- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> +++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> @@ -260,7 +260,7 @@ static int iio_bfin_tmr_trigger_probe(struct
> platform_device *pdev)
> out1:
> iio_trigger_unregister(st->trig);
> out:
> - iio_trigger_put(st->trig);
> + iio_trigger_free(st->trig);
> return ret;
> }
>
> @@ -273,7 +273,7 @@ static int iio_bfin_tmr_trigger_remove(struct
> platform_device *pdev)
> peripheral_free(st->t->pin);
> free_irq(st->irq, st);
> iio_trigger_unregister(st->trig);
> - iio_trigger_put(st->trig);
> + iio_trigger_free(st->trig);
>
> return 0;
> }
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] iio: trigger: free trigger resource correctly
On 01/22/2017 03:25 PM, Jonathan Cameron wrote: > On 20/01/17 03:47, Alison Schofield wrote: >> These stand-alone trigger drivers were using iio_trigger_put() >> where they should have been using iio_trigger_free(). The >> iio_trigger_put() adds a module_put which is bad since they >> never did a module_get. >> >> In the sysfs driver, module_get/put's are used as triggers are >> added & removed. This extra module_put() occurs on an error path >> in the probe routine (probably rare). >> >> In the bfin-timer & interrupt trigger drivers, the module resources >> are not explicitly managed, so it's doing a put on something that >> was never get'd. It occurs on the probe error path and on the >> remove path (not so rare). >> >> Tested with the sysfs trigger driver. >> The bfin & interrupt drivers were build tested & inspected only. >> >> Signed-off-by: Alison Schofield > This is certainly more consistent. > Lars, could you sanity check as well given the bfin timer is > in here. looks good. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] iio: trigger: free trigger resource correctly
On 22/01/17 15:56, Lars-Peter Clausen wrote: > On 01/22/2017 03:25 PM, Jonathan Cameron wrote: >> On 20/01/17 03:47, Alison Schofield wrote: >>> These stand-alone trigger drivers were using iio_trigger_put() >>> where they should have been using iio_trigger_free(). The >>> iio_trigger_put() adds a module_put which is bad since they >>> never did a module_get. >>> >>> In the sysfs driver, module_get/put's are used as triggers are >>> added & removed. This extra module_put() occurs on an error path >>> in the probe routine (probably rare). >>> >>> In the bfin-timer & interrupt trigger drivers, the module resources >>> are not explicitly managed, so it's doing a put on something that >>> was never get'd. It occurs on the probe error path and on the >>> remove path (not so rare). >>> >>> Tested with the sysfs trigger driver. >>> The bfin & interrupt drivers were build tested & inspected only. >>> >>> Signed-off-by: Alison Schofield >> This is certainly more consistent. >> Lars, could you sanity check as well given the bfin timer is >> in here. > > looks good. > Applied to the togreg branch of iio.git. Thought about sending this as a fix, but as it's been there a long time I've taken the view there is no real rush on this. Thanks, Jonathan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH -next] staging: greybus: audio_gb.c: Change uint32_t to u32
Change uint32_t to u32, solved the issue reported by checkpatch.pl:
CHECK: Prefer kernel type 'u32' over 'uint32_t'
+ uint32_t *format, uint32_t *rate, u8 *channels,
CHECK: Prefer kernel type 'u32' over 'uint32_t'
+ uint32_t format, uint32_t rate, u8 channels,
Signed-off-by: Marcos Paulo de Souza
---
drivers/staging/greybus/audio_gb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/audio_gb.c
b/drivers/staging/greybus/audio_gb.c
index 42f287d..7884d84 100644
--- a/drivers/staging/greybus/audio_gb.c
+++ b/drivers/staging/greybus/audio_gb.c
@@ -108,7 +108,7 @@ int gb_audio_gb_disable_widget(struct gb_connection
*connection,
EXPORT_SYMBOL_GPL(gb_audio_gb_disable_widget);
int gb_audio_gb_get_pcm(struct gb_connection *connection, u16 data_cport,
- uint32_t *format, uint32_t *rate, u8 *channels,
+ u32 *format, u32 *rate, u8 *channels,
u8 *sig_bits)
{
struct gb_audio_get_pcm_request req;
@@ -132,7 +132,7 @@ int gb_audio_gb_get_pcm(struct gb_connection *connection,
u16 data_cport,
EXPORT_SYMBOL_GPL(gb_audio_gb_get_pcm);
int gb_audio_gb_set_pcm(struct gb_connection *connection, u16 data_cport,
- uint32_t format, uint32_t rate, u8 channels,
+ u32 format, u32 rate, u8 channels,
u8 sig_bits)
{
struct gb_audio_set_pcm_request req;
--
2.9.3
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] scsi: storvsc: Add support for FC lightweight host.
I'm sorry. In my zeal to push out this patch I have done a poor job of
communication on a number of levels.
The first patch which deals with the fc transport changes will not set
the scsi_transport_template.eh_timed_out function directly during
lightweight fc_attach_transport(). It instead relies on whatever was
indicated as the scsi_host_template timeout handler during
inscsi_times_out() scsi_error.c.
So yes in a sense it is related but now I believe I understand your
point. Perhaps this would fall more under the heading of post
fc_transport implementation storvsc cleanup necessitating its own patch.
I will break it out in the next go round.
Thanks,
Cathy
On 01/20/2017 04:31 AM, Dan Carpenter wrote:
On Thu, Jan 19, 2017 at 12:55:27PM -0500, Cathy Avery wrote:
On 01/18/2017 06:15 PM, Dan Carpenter wrote:
On Wed, Jan 18, 2017 at 03:28:58PM -0500, Cathy Avery wrote:
Enable FC lightweight host option so that the luns exposed by
the driver may be manually scanned.
Signed-off-by: Cathy Avery
---
drivers/scsi/storvsc_drv.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 888e16e..fc1d6ba 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1882,6 +1882,7 @@ static struct hv_driver storvsc_drv = {
static struct fc_function_template fc_transport_functions = {
.show_host_node_name = 1,
.show_host_port_name = 1,
+ .lightweight_transport = 1,
};
#endif
@@ -1906,11 +1907,6 @@ static int __init storvsc_drv_init(void)
fc_transport_template = fc_attach_transport(&fc_transport_functions);
if (!fc_transport_template)
return -ENODEV;
-
- /*
-* Install Hyper-V specific timeout handler.
-*/
- fc_transport_template->eh_timed_out = storvsc_eh_timed_out;
I don't undestand how removing this is related.
Its not related but it is also not necessary so I took it out. The
default scsi timeout handler will be used.
I can certainly put it back.
I'm not sure that we understand each other properly.
Has this patch already been committed? If so, then there is no need to
put it back.
But it if hasn't been committed, can you resend the patches with that
bit broken out into a separate patch with its own changelog? Patches
should only do one thing but you're saying that it's doing two
unrelated things.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] staging: greybus: audio_gb.c: Change uint32_t to u32
On Sun, Jan 22, 2017 at 03:49:21PM -0200, Marcos Paulo de Souza wrote: > Change uint32_t to u32, solved the issue reported by checkpatch.pl: > > CHECK: Prefer kernel type 'u32' over 'uint32_t' > + uint32_t *format, uint32_t *rate, u8 *channels, > > CHECK: Prefer kernel type 'u32' over 'uint32_t' > + uint32_t format, uint32_t rate, u8 channels, > > Signed-off-by: Marcos Paulo de Souza > --- Good catch, thanks. Acked-by: Mark Greer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] scsi: storvsc: Add support for FC lightweight host.
On Sun, Jan 22, 2017 at 01:51:13PM -0500, Cathy Avery wrote: > I'm sorry. In my zeal to push out this patch I have done a poor job > of communication on a number of levels. > > The first patch which deals with the fc transport changes will not > set the scsi_transport_template.eh_timed_out function directly > during lightweight fc_attach_transport(). It instead relies on > whatever was indicated as the scsi_host_template timeout handler > during inscsi_times_out() scsi_error.c. > > So yes in a sense it is related but now I believe I understand your > point. Perhaps this would fall more under the heading of post > fc_transport implementation storvsc cleanup necessitating its own > patch. > > I will break it out in the next go round. Ah... Actually it probably belongs in this patch, it just wasn't obvious to me how they were related (I'm a total newbie with this code so that's probably part of the confusion). Since you're resend the code anyway, could you just add this information to the commit message? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 16/24] media: Add i.MX media core driver
On 01/16/2017 05:47 AM, Philipp Zabel wrote:
On Sat, 2017-01-14 at 14:46 -0800, Steve Longerbeam wrote:
[...]
+Unprocessed Video Capture:
+--
+
+Send frames directly from sensor to camera interface, with no
+conversions:
+
+-> ipu_smfc -> camif
I'd call this capture interface, this is not just for cameras. Or maybe
idmac if you want to mirror hardware names?
Camif is so named because it is the V4L2 user interface for video
capture. I suppose it could be named "capif", but that doesn't role
off the tongue quite as well.
Agreed, capif sounds weird. I find camif a bit confusing though, because
Samsung S3C has a camera interface that is actually called "CAMIF".
how about simply "capture" ?
+ media-ctl -V "\"camif1\":0 [fmt:UYVY2X8/640x480]"
I agree this looks very intuitive, but technically correct for the
csi1:1 and camif1:0 pads would be a 32-bit YUV format.
(MEDIA_BUS_FMT_YUV8_1X32_PADLO doesn't exist yet).
I think it would be better to use the correct format
I'm not sure I follow you here.
The ov5640 sends UYVY2X8 on the wire, so pads "ov5640_mipi 1-0040":0
up to "ipu1_csi1":0 are correct. But the CSI writes 32-bit YUV values
into the SMFC, so the CSI output pad and the IDMAC input pad should have
a YUV8_1X32 format.
Chapter 37.4.2.3 "FCW & FCR - Format converter write and read" in the
IDMAC chapter states that all internal submodules only work on 8-bit per
component formats with four components: YUVA or RGBA.
Right, the "direct" IPU internal (that do not transfer buffers to/from
memory via IDMAC channels) should only allow the IPU internal
formats YUVA and RGBA. I get you now.
The "direct" pads now only accept MEDIA_BUS_FMT_AYUV8_1X32 and
MEDIA_BUS_FMT_ARGB_1X32.
Those pads are:
ipu_csi:1
ipu_vdic:1
ipu_ic_prp:0
ipu_ic_prp:1
ipu_ic_prpenc:0
ipu_ic_prpenc:1
ipu_ic_prpvf:0
ipu_ic_prpvf:1
There does not appear to be support in the FSU for linking a write channel
to the VDIC read channels (8, 9, 10) according to VDI_SRC_SEL field. There
is support for the direct link from CSI (which I am using), but that's
not an
IDMAC channel link.
There is a PRP_SRC_SEL field, with linking from IDMAC (SMFC) channels
0..2 (and 3? it's not clear, and not clear whether this includes channel 1).
As I read it, that is 0 and 2 only, no idea why. But since there are
only 2 CSIs, that shouldn't be a problem.
ipu_csi1 is now transferring on IDMAC/SMFC channel 2 (ipu_csi0 still
at channel 0).
+static const u32 power_off_seq[] = {
+ IMX_MEDIA_GRP_ID_IC_PP,
+ IMX_MEDIA_GRP_ID_IC_PRPVF,
+ IMX_MEDIA_GRP_ID_IC_PRPENC,
+ IMX_MEDIA_GRP_ID_SMFC,
+ IMX_MEDIA_GRP_ID_CSI,
+ IMX_MEDIA_GRP_ID_VIDMUX,
+ IMX_MEDIA_GRP_ID_SENSOR,
+ IMX_MEDIA_GRP_ID_CSI2,
+};
This seems somewhat arbitrary. Why is a power sequence needed?
The CSI-2 receiver must be powered up before the sensor, that's the
only requirement IIRC. The others have no s_power requirement. So I
can probably change this to power up in the frontend -> backend order
(IC_PP to sensor). And vice-versa for power off.
Yes, I think that should work (see below).
Actually there are problems using this, see below.
[...]
+/*
+ * Turn current pipeline power on/off starting from start_entity.
+ * Must be called with mdev->graph_mutex held.
+ */
+int imx_media_pipeline_set_power(struct imx_media_dev *imxmd,
+struct media_entity_graph *graph,
+struct media_entity *start_entity, bool on)
+{
+ struct media_entity *entity;
+ struct v4l2_subdev *sd;
+ int i, ret = 0;
+ u32 id;
+
+ for (i = 0; i < NUM_POWER_ENTITIES; i++) {
+ id = on ? power_on_seq[i] : power_off_seq[i];
+ entity = find_pipeline_entity(imxmd, graph, start_entity, id);
+ if (!entity)
+ continue;
+
+ sd = media_entity_to_v4l2_subdev(entity);
+
+ ret = v4l2_subdev_call(sd, core, s_power, on);
+ if (ret && ret != -ENOIOCTLCMD)
+ break;
+ }
+
+ return (ret && ret != -ENOIOCTLCMD) ? ret : 0;
+}
+EXPORT_SYMBOL_GPL(imx_media_pipeline_set_power);
This should really be handled by v4l2_pipeline_pm_use.
I thought about this earlier, but v4l2_pipeline_pm_use() seems to be
doing some other stuff that bothered me, at least that's what I remember.
I will revisit this.
I have used it with a tc358743 -> mipi-csi2 pipeline, it didn't cause
any problems. It would be better to reuse and, if necessary, fix the
existing infrastructure where available.
I tried this API, by switching to v4l2_pipeline_pm_use() in camif
open/release,
and switched to v4l2_pipeline_link_notify() instead of
imx_media_link_notify()
in the media driver's media_device_ops.
This API assumes the video device has an open file handle while the media
links are being established. This doesn't work for me, I want to be able to
establish the
Re: [PATCH 2/2] scsi: storvsc: Add support for FC lightweight host.
On Wed, 01/18 15:28, Cathy Avery wrote: > Enable FC lightweight host option so that the luns exposed by > the driver may be manually scanned. Hi Cathy, out of curiosity: how does this relate to issue_lip operation? And how to trigger manual scan with this patch? Fam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] staging: greybus: audio_gb.c: Change uint32_t to u32
On Sun, Jan 22, 2017 at 11:19 PM, Marcos Paulo de Souza wrote: > Change uint32_t to u32, solved the issue reported by checkpatch.pl: > > CHECK: Prefer kernel type 'u32' over 'uint32_t' > + uint32_t *format, uint32_t *rate, u8 *channels, > > CHECK: Prefer kernel type 'u32' over 'uint32_t' > + uint32_t format, uint32_t rate, u8 channels, > > Signed-off-by: Marcos Paulo de Souza > --- Acked-by: Vaibhav Agarwal ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
