Re: [GIT PULL FOR 3.6] V4L2 API cleanups
Hi Sakari, On Monday 11 June 2012 12:39:44 Sakari Ailus wrote: > On Mon, Jun 11, 2012 at 09:50:54AM +0200, Laurent Pinchart wrote: > > On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote: > > > Hi Mauro, > > > > > > Here are two V4L2 API cleanup patches; the first removes __user from > > > videodev2.h from a few places, making it possible to use the header file > > > as such in user space, while the second one changes the > > > v4l2_buffer.input field back to reserved. > > > > > > The following changes since commit 5472d3f17845c4398c6a510b46855820920c2181: > > > [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24 > > > > > > 09:27:24 -0300) > > > > > > are available in the git repository at: > > > ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6 > > > > > > Sakari Ailus (2): > > > v4l: Remove __user from interface structure definitions > > > > NAK, sorry. > > > > __user has a purpose, we need to add it where it's missing, not remove it > > where it's rightfully present. > > It's not quite as simple as adding __user everywhere it might belong to --- > these structs are being used in kernel space, too. The structs that are part > of the user space interface may at some point contain pointers to memory > which is in user space. That is being dealt by video_usercopy(), so the > individual drivers or the rest of the V4L2 framework always gets pointers > pointing to kernel memory. Very good point, I haven't thought about that. I'm not sure how to deal with this, splitting structures in a __user and a non __user version isn't really a good option. Maybe the sparse tool should be somehow extended ? > These particular fields aren't handled by the framework currently, so > removing __user there requires adding the support to video_usercopy(). -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dvb_usb_v2: use pointers to properties[REGRESSION]
On 06/16/2012 11:17 PM, Malcolm Priestley wrote:
On Sat, 2012-06-16 at 21:41 +0300, Antti Palosaari wrote:
That is what you want to do:
CALLBACK(struct dvb_usb_adapter *adap)
{
struct dvb_frontend *fe = adap->fe[adap->active_fe];
// now we have pointer to adap and fe
}
That is what I want to do:
**
CALLBACK(struct dvb_frontend *fe)
{
struct dvb_usb_adapter *adap = fe->dvb->priv;
// now we have pointer to adap and fe
}
I just don't like the idea of deliberately sending a NULL object to
a callback.
Same here, I mentioned that many times I will fix it. And it is now
fixed, just fetch latest changes.
Ha ... I know what is causing the crashits in usb_urb.c
int usb_urb_init(struct usb_data_stream *stream,
struct usb_data_stream_properties *props)
{
int ret;
if (stream == NULL || props == NULL)
return -EINVAL;
memcpy(&stream->props, props, sizeof(*props));
usb_clear_halt(stream->udev, usb_rcvbulkpipe(stream->udev,
stream->props.endpoint));
The usb_clear_halt with 0 endpoint.
It can tweaked by sending a valid endpoint.
Aaah, that explains. Anyhow, I am almost sure usb_clear_halt() will not
crash but return error in worst case. You are likely using that endpoint
in your driver elsewhere and it is crashing as next operation to
endpoint fails. Error in some USB control routines of your driver? It
could be also error handling error in dvb usb routines, the idea is to
stop device registration and un-register all if there is error. But I
haven't tested all the error branches.
Anyhow, check recent patches and I think you are happy.
regards
Antti
--
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dvb_usb_v2: use pointers to properties[REGRESSION]
On Sat, 2012-06-16 at 21:41 +0300, Antti Palosaari wrote:
>
> That is what you want to do:
>
> CALLBACK(struct dvb_usb_adapter *adap)
> {
>struct dvb_frontend *fe = adap->fe[adap->active_fe];
>// now we have pointer to adap and fe
> }
>
> That is what I want to do:
> **
> CALLBACK(struct dvb_frontend *fe)
> {
>struct dvb_usb_adapter *adap = fe->dvb->priv;
>// now we have pointer to adap and fe
> }
I just don't like the idea of deliberately sending a NULL object to
a callback.
Ha ... I know what is causing the crashits in usb_urb.c
int usb_urb_init(struct usb_data_stream *stream,
struct usb_data_stream_properties *props)
{
int ret;
if (stream == NULL || props == NULL)
return -EINVAL;
memcpy(&stream->props, props, sizeof(*props));
usb_clear_halt(stream->udev, usb_rcvbulkpipe(stream->udev,
stream->props.endpoint));
The usb_clear_halt with 0 endpoint.
It can tweaked by sending a valid endpoint.
Regards
Malcolm
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dvb_usb_v2: use pointers to properties[REGRESSION]
On 06/16/2012 09:12 PM, Malcolm Priestley wrote:
On Sat, 2012-06-16 at 18:52 +0300, Antti Palosaari wrote:
On 06/16/2012 03:06 PM, Malcolm Priestley wrote:
On Sat, 2012-06-16 at 04:16 +0300, Antti Palosaari wrote:
I did example for you, here you are :)
On 06/16/2012 03:55 AM, Antti Palosaari wrote:
On 06/16/2012 03:35 AM, Malcolm Priestley wrote:
On Sat, 2012-06-16 at 01:54 +0300, Antti Palosaari wrote:
Hello Malcolm,
On 06/16/2012 01:11 AM, Malcolm Priestley wrote:
Hi Antti
You can't have dvb_usb_device_properties as constant structure pointer.
At run time it needs to be copied to a private area.
Having constant structure for properties was one of main idea of whole
change. Earlier it causes some problems when driver changes those values
- for example remote configuration based info from the eeprom.
Two or more devices of the same type on the system will be pointing to
the same structure.
Yes and no. You can define struct dvb_usb_device_properties for each
USB ID.
Any changes they make to the structure will be common to all.
For those devices having same USB ID only.
Changing dvb_usb_device_properties is *not* allowed. It is constant and
should be. That was how I designed it. Due to that I introduced those
new callbacks to resolve needed values dynamically.
Yes, but it does make run-time tweaks difficult.
If there is still something that is needed to resolve at runtime I am
happy to add new callback. For example PID filter configuration is
static currently as per adapter and if it is needed to to reconfigure at
runtime new callback is needed.
I will look at the PID filter later, it defaulted to off.
However, in my builds for ARM devices it is defaulted on. I will be
testing this later. I can't see any problems.
Could you say what is your problem I can likely say how to resolve it.
Well, the problem is, I now need two separate structures for LME2510 and
LME2510C as in the existing driver, the hope was to merge them as one.
The only difference being the stream endpoint number.
Then you should use .get_usb_stream_config() instead of static stream
configuration. That is now supported.
I have found one logical error in my current implementation.
get_usb_stream_config gets frontend as a parameter, but it is called
frontend == NULL when attaching adapters and after that frontend is real
value when called (just before streaming is started). Now what happens
if we has multiple adapters? We cannot know which adapter is requested
during attach since FE is NULL :)
But that is problem case only if you have multiple adapters. I will find
out better solution next few days. It is ugly now.
You can just skip fe checking or something. See AF9015 for example.
Currently, it is implemented in identify_state on dvb_usb_v2.
The get_usb_stream_config has no access to device to to allow a run-time
change there.
Hmmm, what you try to say? As FE is given as a pointer, you have also
adapter and device. Those are nested, device is root, then adapter is
under that and finally frontend are under the adapter.
.
└── device
├── adapter0
│ ├── frontend0
│ ├── frontend1
│ └── frontend2
└── adapter1
├── frontend0
├── frontend1
└── frontend2
regards
Antti
static int lme2510_get_usb_stream_config(struct dvb_frontend *fe,
struct usb_data_stream_properties *stream)
{
struct dvb_usb_adapter *adap;
struct lme2510_state *state;
stream->type = USB_BULK;
stream->count = 10;
stream->u.bulk.buffersize = 4096;
// ugly part begins
if (fe == NULL)
return 0;
adap = fe->dvb->priv;
state = adap->dev->priv;
// ugly part ends
if (state->chip_id == lme2510c)
stream->endpoint = 8;
else
stream->endpoint = 6;
return 0;
}
Ugly part between comments is what I am going to change, but currently
it works as is.
This doesn't work because the code will return the stream->endpoint
unset and then try to initialise usb_urb_init.
So what? It happens only once when device is initialized. We do not need
to know endpoint at that phase as we do not stream yet.
Please read your code you submit the urbs.
int dvb_usb_adapter_stream_init(struct dvb_usb_adapter *adap)
{
int ret;
struct usb_data_stream_properties stream_props;
adap->stream.udev = adap->dev->udev;
adap->stream.user_priv = adap;
/* resolve USB stream configuration for buffer alloc */
if (adap->dev->props->get_usb_stream_config) {
ret = adap->dev->props->get_usb_stream_config(NULL,
&stream_props);
if (ret < 0)
return ret;
} else {
stream_props = adap->props->stream;
}
/* FIXME: can be removed as set later in anyway */
adap->stream.complete = dvb_usb_data_com
Re: dvb_usb_v2: use pointers to properties[REGRESSION]
On Sat, 2012-06-16 at 18:52 +0300, Antti Palosaari wrote:
> On 06/16/2012 03:06 PM, Malcolm Priestley wrote:
> > On Sat, 2012-06-16 at 04:16 +0300, Antti Palosaari wrote:
> >> I did example for you, here you are :)
> >>
> >> On 06/16/2012 03:55 AM, Antti Palosaari wrote:
> >>> On 06/16/2012 03:35 AM, Malcolm Priestley wrote:
> On Sat, 2012-06-16 at 01:54 +0300, Antti Palosaari wrote:
> > Hello Malcolm,
> >
> > On 06/16/2012 01:11 AM, Malcolm Priestley wrote:
> >> Hi Antti
> >>
> >> You can't have dvb_usb_device_properties as constant structure pointer.
> >>
> >> At run time it needs to be copied to a private area.
> >
> > Having constant structure for properties was one of main idea of whole
> > change. Earlier it causes some problems when driver changes those values
> > - for example remote configuration based info from the eeprom.
> >
> >> Two or more devices of the same type on the system will be pointing to
> >> the same structure.
> >
> > Yes and no. You can define struct dvb_usb_device_properties for each
> > USB ID.
> >
> >> Any changes they make to the structure will be common to all.
> >
> > For those devices having same USB ID only.
> > Changing dvb_usb_device_properties is *not* allowed. It is constant and
> > should be. That was how I designed it. Due to that I introduced those
> > new callbacks to resolve needed values dynamically.
> Yes, but it does make run-time tweaks difficult.
>
> > If there is still something that is needed to resolve at runtime I am
> > happy to add new callback. For example PID filter configuration is
> > static currently as per adapter and if it is needed to to reconfigure at
> > runtime new callback is needed.
> I will look at the PID filter later, it defaulted to off.
>
> However, in my builds for ARM devices it is defaulted on. I will be
> testing this later. I can't see any problems.
>
> >
> > Could you say what is your problem I can likely say how to resolve it.
> >
>
> Well, the problem is, I now need two separate structures for LME2510 and
> LME2510C as in the existing driver, the hope was to merge them as one.
> The only difference being the stream endpoint number.
> >>>
> >>> Then you should use .get_usb_stream_config() instead of static stream
> >>> configuration. That is now supported.
> >>>
> >>> I have found one logical error in my current implementation.
> >>> get_usb_stream_config gets frontend as a parameter, but it is called
> >>> frontend == NULL when attaching adapters and after that frontend is real
> >>> value when called (just before streaming is started). Now what happens
> >>> if we has multiple adapters? We cannot know which adapter is requested
> >>> during attach since FE is NULL :)
> >>> But that is problem case only if you have multiple adapters. I will find
> >>> out better solution next few days. It is ugly now.
> >>> You can just skip fe checking or something. See AF9015 for example.
> >>>
> Currently, it is implemented in identify_state on dvb_usb_v2.
>
> The get_usb_stream_config has no access to device to to allow a run-time
> change there.
> >>>
> >>> Hmmm, what you try to say? As FE is given as a pointer, you have also
> >>> adapter and device. Those are nested, device is root, then adapter is
> >>> under that and finally frontend are under the adapter.
> >>>
> >>>
> >>> .
> >>> └── device
> >>> ├── adapter0
> >>> │ ├── frontend0
> >>> │ ├── frontend1
> >>> │ └── frontend2
> >>> └── adapter1
> >>> ├── frontend0
> >>> ├── frontend1
> >>> └── frontend2
> >>>
> >>>
> >>> regards
> >>> Antti
> >>
> >> static int lme2510_get_usb_stream_config(struct dvb_frontend *fe,
> >>struct usb_data_stream_properties *stream)
> >> {
> >>struct dvb_usb_adapter *adap;
> >>struct lme2510_state *state;
> >>
> >>stream->type = USB_BULK;
> >>stream->count = 10;
> >>stream->u.bulk.buffersize = 4096;
> >>
> >> // ugly part begins
> >>if (fe == NULL)
> >>return 0;
> >>adap = fe->dvb->priv;
> >>state = adap->dev->priv;
> >> // ugly part ends
> >>
> >>if (state->chip_id == lme2510c)
> >>stream->endpoint = 8;
> >>else
> >>stream->endpoint = 6;
> >>
> >>return 0;
> >> }
> >>
> >> Ugly part between comments is what I am going to change, but currently
> >> it works as is.
> >>
> > This doesn't work because the code will return the stream->endpoint
> > unset and then try to initialise usb_urb_init.
>
> So what? It happens only once when device is initialized. We do not need
> to know endpoint at that phase as we do not stream yet.
Please read your code you submit the urbs.
int dvb_usb_adapter_stream_init(struct dvb_usb_adapter *adap)
{
int ret;
struct usb_data_stream_properties stream_pr
cron job: media_tree daily build: WARNINGS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date:Sat Jun 16 19:00:20 CEST 2012 git hash:5472d3f17845c4398c6a510b46855820920c2181 gcc version: i686-linux-gcc (GCC) 4.6.3 host hardware:x86_64 host os: 3.3-6.slh.2-amd64 linux-git-arm-eabi-davinci: WARNINGS linux-git-arm-eabi-exynos: WARNINGS linux-git-arm-eabi-omap: WARNINGS linux-git-i686: WARNINGS linux-git-m32r: WARNINGS linux-git-mips: WARNINGS linux-git-powerpc64: WARNINGS linux-git-x86_64: WARNINGS linux-2.6.31.12-i686: WARNINGS linux-2.6.32.6-i686: WARNINGS linux-2.6.33-i686: WARNINGS linux-2.6.34-i686: WARNINGS linux-2.6.35.3-i686: WARNINGS linux-2.6.36-i686: WARNINGS linux-2.6.37-i686: WARNINGS linux-2.6.38.2-i686: WARNINGS linux-2.6.39.1-i686: WARNINGS linux-3.0-i686: WARNINGS linux-3.1-i686: WARNINGS linux-3.2.1-i686: WARNINGS linux-3.3-i686: WARNINGS linux-3.4-i686: WARNINGS linux-2.6.31.12-x86_64: WARNINGS linux-2.6.32.6-x86_64: WARNINGS linux-2.6.33-x86_64: WARNINGS linux-2.6.34-x86_64: WARNINGS linux-2.6.35.3-x86_64: WARNINGS linux-2.6.36-x86_64: WARNINGS linux-2.6.37-x86_64: WARNINGS linux-2.6.38.2-x86_64: WARNINGS linux-2.6.39.1-x86_64: WARNINGS linux-3.0-x86_64: WARNINGS linux-3.1-x86_64: WARNINGS linux-3.2.1-x86_64: WARNINGS linux-3.3-x86_64: WARNINGS linux-3.4-x86_64: WARNINGS apps: WARNINGS spec-git: WARNINGS sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.tar.bz2 The V4L-DVB specification from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L/DVB (12730): Add conexant cx25821 driver
Hi everyone,
On Sat, Jun 16, 2012 at 10:35 AM, Dan Carpenter
wrote:
>
> Hm... There are several more places which have this same problem.
> I'm not sure what's going on here.
>
> drivers/media/video/saa7164/saa7164-i2c.c:112 saa7164_i2c_register() error:
> memcpy() '&saa7164_i2c_algo_template' too small (24 vs 64)
I was just looking at that lines in saa7164_i2c_register:
112 memcpy(&bus->i2c_algo, &saa7164_i2c_algo_template,
113sizeof(bus->i2c_algo));
They seem like pointless to me. The real algo is set here:
93 static struct i2c_adapter saa7164_i2c_adap_template = {
94 .name = "saa7164",
95 .owner = THIS_MODULE,
96 .algo = &saa7164_i2c_algo_template,
97 };
This would also mean that this fields are also pointless:
254 struct i2c_algo_bit_datai2c_algo;
255 struct i2c_client i2c_client;
IMO, the issue pointed out by Dan would never appeared
if instead of using memcpy to fill the structures, it would just
get assigned; it's type safe, right?
Please correct me if I'm wrong,
Ezequiel.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: V4L/DVB (12730): Add conexant cx25821 driver
Thanks Dan. Let me take a look and get back to you. Rgds, Palash From: Dan Carpenter [dan.carpen...@oracle.com] Sent: Saturday, June 16, 2012 6:16 AM To: Palash Bandyopadhyay; mche...@redhat.com Cc: linux-media@vger.kernel.org Subject: re: V4L/DVB (12730): Add conexant cx25821 driver Hello Palash Bandyopadhyay, The patch 02b20b0b4cde: "V4L/DVB (12730): Add conexant cx25821 driver" from Sep 15, 2009, leads to the following warning: drivers/media/video/cx25821/cx25821-i2c.c:310 cx25821_i2c_register() error: memcpy() '&cx25821_i2c_algo_template' too small (24 vs 64) 306 dprintk(1, "%s(bus = %d)\n", __func__, bus->nr); 307 308 memcpy(&bus->i2c_adap, &cx25821_i2c_adap_template, 309 sizeof(bus->i2c_adap)); > 310 memcpy(&bus->i2c_algo, &cx25821_i2c_algo_template, 311 sizeof(bus->i2c_algo)); 312 memcpy(&bus->i2c_client, &cx25821_i2c_client_template, 313 sizeof(bus->i2c_client)); 314 The problem is that "bus->i2c_algo" is a i2c_algo_bit_data struct and cx25821_i2c_algo_template is a i2c_algorithm struct. They are different sizes and the function pointers don't line up at all. I don't see how this can work at all. regards, dan carpenter Conexant E-mail Firewall (Conexant.Com) made the following annotations - ** Legal Disclaimer "This email may contain confidential and privileged material for the sole use of the intended recipient. Any unauthorized review, use or distribution by others is strictly prohibited. If you have received the message in error, please advise the sender by reply email and delete the message. Thank you." ** - -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dvb_usb_v2: use pointers to properties[REGRESSION]
On 06/16/2012 03:06 PM, Malcolm Priestley wrote:
On Sat, 2012-06-16 at 04:16 +0300, Antti Palosaari wrote:
I did example for you, here you are :)
On 06/16/2012 03:55 AM, Antti Palosaari wrote:
On 06/16/2012 03:35 AM, Malcolm Priestley wrote:
On Sat, 2012-06-16 at 01:54 +0300, Antti Palosaari wrote:
Hello Malcolm,
On 06/16/2012 01:11 AM, Malcolm Priestley wrote:
Hi Antti
You can't have dvb_usb_device_properties as constant structure pointer.
At run time it needs to be copied to a private area.
Having constant structure for properties was one of main idea of whole
change. Earlier it causes some problems when driver changes those values
- for example remote configuration based info from the eeprom.
Two or more devices of the same type on the system will be pointing to
the same structure.
Yes and no. You can define struct dvb_usb_device_properties for each
USB ID.
Any changes they make to the structure will be common to all.
For those devices having same USB ID only.
Changing dvb_usb_device_properties is *not* allowed. It is constant and
should be. That was how I designed it. Due to that I introduced those
new callbacks to resolve needed values dynamically.
Yes, but it does make run-time tweaks difficult.
If there is still something that is needed to resolve at runtime I am
happy to add new callback. For example PID filter configuration is
static currently as per adapter and if it is needed to to reconfigure at
runtime new callback is needed.
I will look at the PID filter later, it defaulted to off.
However, in my builds for ARM devices it is defaulted on. I will be
testing this later. I can't see any problems.
Could you say what is your problem I can likely say how to resolve it.
Well, the problem is, I now need two separate structures for LME2510 and
LME2510C as in the existing driver, the hope was to merge them as one.
The only difference being the stream endpoint number.
Then you should use .get_usb_stream_config() instead of static stream
configuration. That is now supported.
I have found one logical error in my current implementation.
get_usb_stream_config gets frontend as a parameter, but it is called
frontend == NULL when attaching adapters and after that frontend is real
value when called (just before streaming is started). Now what happens
if we has multiple adapters? We cannot know which adapter is requested
during attach since FE is NULL :)
But that is problem case only if you have multiple adapters. I will find
out better solution next few days. It is ugly now.
You can just skip fe checking or something. See AF9015 for example.
Currently, it is implemented in identify_state on dvb_usb_v2.
The get_usb_stream_config has no access to device to to allow a run-time
change there.
Hmmm, what you try to say? As FE is given as a pointer, you have also
adapter and device. Those are nested, device is root, then adapter is
under that and finally frontend are under the adapter.
.
└── device
├── adapter0
│ ├── frontend0
│ ├── frontend1
│ └── frontend2
└── adapter1
├── frontend0
├── frontend1
└── frontend2
regards
Antti
static int lme2510_get_usb_stream_config(struct dvb_frontend *fe,
struct usb_data_stream_properties *stream)
{
struct dvb_usb_adapter *adap;
struct lme2510_state *state;
stream->type = USB_BULK;
stream->count = 10;
stream->u.bulk.buffersize = 4096;
// ugly part begins
if (fe == NULL)
return 0;
adap = fe->dvb->priv;
state = adap->dev->priv;
// ugly part ends
if (state->chip_id == lme2510c)
stream->endpoint = 8;
else
stream->endpoint = 6;
return 0;
}
Ugly part between comments is what I am going to change, but currently
it works as is.
This doesn't work because the code will return the stream->endpoint
unset and then try to initialise usb_urb_init.
So what? It happens only once when device is initialized. We do not need
to know endpoint at that phase as we do not stream yet.
It would be far better to have get_usb_stream_config point to adapter
and point to adap->fe[adap->active_fe] inside if need be.
Why?
As we are going to configure stream based _frontend_ current needs we
just need to know frontend and nothing more or less. If you pass adapter
as a parameter then you don't know what is frontend in question as
adapter coould have multiple frontends.
active_fe member variable is meant to be internal use of DVB USB
framework. You should not use it from the driver. You can see active
frontend id just looking current frontend id (fe->id).
This is also the case with af9015, the code is extracting adapter
through another module.
I don't understand what you mean.
Also I would like to ask if you test code I given at all?
You seems not to understand that callback - get_usb_stream_config() - is
there for configuring st
Re: V4L/DVB (12730): Add conexant cx25821 driver
Hm... There are several more places which have this same problem. I'm not sure what's going on here. drivers/media/video/saa7164/saa7164-i2c.c:112 saa7164_i2c_register() error: memcpy() '&saa7164_i2c_algo_template' too small (24 vs 64) drivers/media/video/cx23885/cx23885-i2c.c:321 cx23885_i2c_register() error: memcpy() '&cx23885_i2c_algo_template' too small (24 vs 64) drivers/media/video/cx231xx/cx231xx-i2c.c:503 cx231xx_i2c_register() error: memcpy() '&cx231xx_algo' too small (24 vs 64) regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
re: V4L/DVB (12730): Add conexant cx25821 driver
Hello Palash Bandyopadhyay, The patch 02b20b0b4cde: "V4L/DVB (12730): Add conexant cx25821 driver" from Sep 15, 2009, leads to the following warning: drivers/media/video/cx25821/cx25821-i2c.c:310 cx25821_i2c_register() error: memcpy() '&cx25821_i2c_algo_template' too small (24 vs 64) 306 dprintk(1, "%s(bus = %d)\n", __func__, bus->nr); 307 308 memcpy(&bus->i2c_adap, &cx25821_i2c_adap_template, 309 sizeof(bus->i2c_adap)); > 310 memcpy(&bus->i2c_algo, &cx25821_i2c_algo_template, 311 sizeof(bus->i2c_algo)); 312 memcpy(&bus->i2c_client, &cx25821_i2c_client_template, 313 sizeof(bus->i2c_client)); 314 The problem is that "bus->i2c_algo" is a i2c_algo_bit_data struct and cx25821_i2c_algo_template is a i2c_algorithm struct. They are different sizes and the function pointers don't line up at all. I don't see how this can work at all. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dvb_usb_v2: use pointers to properties[REGRESSION]
On Sat, 2012-06-16 at 04:16 +0300, Antti Palosaari wrote:
> I did example for you, here you are :)
>
> On 06/16/2012 03:55 AM, Antti Palosaari wrote:
> > On 06/16/2012 03:35 AM, Malcolm Priestley wrote:
> >> On Sat, 2012-06-16 at 01:54 +0300, Antti Palosaari wrote:
> >>> Hello Malcolm,
> >>>
> >>> On 06/16/2012 01:11 AM, Malcolm Priestley wrote:
> Hi Antti
>
> You can't have dvb_usb_device_properties as constant structure pointer.
>
> At run time it needs to be copied to a private area.
> >>>
> >>> Having constant structure for properties was one of main idea of whole
> >>> change. Earlier it causes some problems when driver changes those values
> >>> - for example remote configuration based info from the eeprom.
> >>>
> Two or more devices of the same type on the system will be pointing to
> the same structure.
> >>>
> >>> Yes and no. You can define struct dvb_usb_device_properties for each
> >>> USB ID.
> >>>
> Any changes they make to the structure will be common to all.
> >>>
> >>> For those devices having same USB ID only.
> >>> Changing dvb_usb_device_properties is *not* allowed. It is constant and
> >>> should be. That was how I designed it. Due to that I introduced those
> >>> new callbacks to resolve needed values dynamically.
> >> Yes, but it does make run-time tweaks difficult.
> >>
> >>> If there is still something that is needed to resolve at runtime I am
> >>> happy to add new callback. For example PID filter configuration is
> >>> static currently as per adapter and if it is needed to to reconfigure at
> >>> runtime new callback is needed.
> >> I will look at the PID filter later, it defaulted to off.
> >>
> >> However, in my builds for ARM devices it is defaulted on. I will be
> >> testing this later. I can't see any problems.
> >>
> >>>
> >>> Could you say what is your problem I can likely say how to resolve it.
> >>>
> >>
> >> Well, the problem is, I now need two separate structures for LME2510 and
> >> LME2510C as in the existing driver, the hope was to merge them as one.
> >> The only difference being the stream endpoint number.
> >
> > Then you should use .get_usb_stream_config() instead of static stream
> > configuration. That is now supported.
> >
> > I have found one logical error in my current implementation.
> > get_usb_stream_config gets frontend as a parameter, but it is called
> > frontend == NULL when attaching adapters and after that frontend is real
> > value when called (just before streaming is started). Now what happens
> > if we has multiple adapters? We cannot know which adapter is requested
> > during attach since FE is NULL :)
> > But that is problem case only if you have multiple adapters. I will find
> > out better solution next few days. It is ugly now.
> > You can just skip fe checking or something. See AF9015 for example.
> >
> >> Currently, it is implemented in identify_state on dvb_usb_v2.
> >>
> >> The get_usb_stream_config has no access to device to to allow a run-time
> >> change there.
> >
> > Hmmm, what you try to say? As FE is given as a pointer, you have also
> > adapter and device. Those are nested, device is root, then adapter is
> > under that and finally frontend are under the adapter.
> >
> >
> > .
> > └── device
> > ├── adapter0
> > │ ├── frontend0
> > │ ├── frontend1
> > │ └── frontend2
> > └── adapter1
> > ├── frontend0
> > ├── frontend1
> > └── frontend2
> >
> >
> > regards
> > Antti
>
> static int lme2510_get_usb_stream_config(struct dvb_frontend *fe,
> struct usb_data_stream_properties *stream)
> {
> struct dvb_usb_adapter *adap;
> struct lme2510_state *state;
>
> stream->type = USB_BULK;
> stream->count = 10;
> stream->u.bulk.buffersize = 4096;
>
> // ugly part begins
> if (fe == NULL)
> return 0;
> adap = fe->dvb->priv;
> state = adap->dev->priv;
> // ugly part ends
>
> if (state->chip_id == lme2510c)
> stream->endpoint = 8;
> else
> stream->endpoint = 6;
>
> return 0;
> }
>
> Ugly part between comments is what I am going to change, but currently
> it works as is.
>
This doesn't work because the code will return the stream->endpoint
unset and then try to initialise usb_urb_init.
It would be far better to have get_usb_stream_config point to adapter
and point to adap->fe[adap->active_fe] inside if need be.
This is also the case with af9015, the code is extracting adapter
through another module.
Regards
Malcolm
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
