Skip to contentSkip to conversation

modprobing 'vidtv' doesn't really do anything

Linux-kernel-mentees Daniel W. S. Almeida 2 participants 3 messages May 19, 2020 – May 20, 2020 1 message not in the archive Media (V4L/DVB)

lore
Share & tools
lore

Daniel W. S. Almeida

author
#1 lore raw
In reply to a message not in the archive

With your current patch the probing code is actually called now, Mauro. Thanks!

As for the sparse errors you've pointed out earlier, I'm at a loss. Yes, I have noticed them, but let's look at an example (they're all mostly the same)

Quoted 3 lines

SPARSE:drivers/media/test-drivers/vidtv/vidtv_psi.c drivers/media/test-drivers/vidtv/vidtv_psi.c:174:43: warning: Using plain integer as NULL pointer

This actually refers to this line:

Quoted 1 line

struct psi_write_args psi_args = {0}

Which seems to me like a valid way to zero-initialize a struct in C?

Next up is...

Quoted 3 lines

SPARSE:drivers/media/test-drivers/vidtv/vidtv_pes.c drivers/media/test-drivers/vidtv/vidtv_pes.c:80:54: warning: missing braces around initializer

I assume this refers to the following line, which is the same deal as above.

Quoted 1 line

struct vidtv_pes_optional_pts pts = {0};

And then there's this, which is an actual mistake. I have mostly eliminated these, but this guy has slipped by.

Quoted 3 lines

SPARSE:drivers/media/test-drivers/vidtv/vidtv_s302m.c drivers/media/test-drivers/vidtv/vidtv_s302m.c:430:36: warning:
incorrect type in assignment (different base types)

Just one more thing. This change on vidtv_bridge.c:

Quoted 16 lines

vidtv_bridge_check_demod_lock(struct vidtv_dvb *dvb, u32 n)

dvb->fe[n]->ops.read_status(dvb->fe[n], &status);

(quoted diff) +7 -5
-	return status == (FE_HAS_SIGNAL |
-	FE_HAS_CARRIER |
-	FE_HAS_VITERBI |
-	FE_HAS_SYNC |
-	FE_HAS_LOCK);
+	status = FE_HAS_SIGNAL |
+	FE_HAS_CARRIER |
+	FE_HAS_VITERBI |
+	FE_HAS_SYNC |
+	FE_HAS_LOCK;
+
+	return status;
Quoted 1 line

}

I did not understand that. Why was the boolean expression replaced by an assignment? This was so eventually we could drop packets or simulate some sort of noise in the event that one of these flags was not set, as we've discussed at some point.

Thanks,

- Daniel

author
#3 lore raw
In reply to Mauro Carvalho Chehab

Hi Mauro,

Quoted 5 lines

Actually, passing 0 is not the right thing there. The init code should either be using gcc style:

	struct psi_write_args psi_args = {}

I did not know that! Now fixed by replacing " = {0}" with " = {}"

As I said, the probing functions are actually called, but now I am running into other issues..

I have sent a new revision (v6), in which I have squashed your patch plus a few changes to the bridge driver.

In vidtv_bridge.c:266, dvb_module_probe always returns NULL. That is because i2c_client_has_driver(client) fails, because client->dev.driver is NULL.

I suspect this line is to blame, but I am not sure..

Quoted 1 line
	i2c_adapter->dev.parent = &dvb->pdev->dev;

Also, when this happens, some error handling code will run starting at vidtv_bridge.c:383, but that will not remove the modules (i.e. lsmod will still list the bridge and the demod). Is this expected? Should I call vidtv_bridge_exit manually?

Also, should I worry about this?

Quoted 2 lines
BUG: KASAN: user-memory-access in >do_init_module+0x1d/0x330
Read of size 8 at addr 000000008322fe90 by task >modprobe/1290

I did not call this directly and I don't see a way to trace it back to my code just by looking at "do_init_module+0x1d/0x330"

Thanks

- Daniel

maintainer
#2 lore raw
In reply to Daniel W. S. Almeida

Em Tue, 19 May 2020 04:13:07 -0300
"Daniel W. S. Almeida" <[email protected]> escreveu:

Quoted 10 lines

With your current patch the probing code is actually called now, Mauro. Thanks!

As for the sparse errors you've pointed out earlier, I'm at a loss. Yes, I have noticed them, but let's look at an example (they're all mostly the same)

Quoted 3 lines

SPARSE:drivers/media/test-drivers/vidtv/vidtv_psi.c drivers/media/test-drivers/vidtv/vidtv_psi.c:174:43: warning: Using plain integer as NULL pointer

Quoted 5 lines

This actually refers to this line:

Quoted 1 line

struct psi_write_args psi_args = {0}

Which seems to me like a valid way to zero-initialize a struct in C?

Actually, passing 0 is not the right thing there. The init code should either be using gcc style:

	struct psi_write_args psi_args = {}

or something where the first argument matches the first argument of the struct, as, according with the C99 specs, doing something like:

	struct foo {
		void *bar;
		...
	};

	struct psi_write_args psi_args = {NULL};

Would do is to use NULL to initialize "bar" variable.

See comments about that at https://stackoverflow.com/questions/11152160/initializing-a-struct-to-0:

	"Reference C99 Standard 6.7.8.21:

	If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration."

The other values would be initialized with the "same as objects that have static storage", e. g. with zero.

In other words, you need to check what's the first element of the struct, when using C99 initialization. E. g, if the first element is a pointer, the init should be:

	struct some_struct_that_starts_with_a_pointer = {NULL};

Tricky.

Quoted 36 lines

Next up is...

Quoted 3 lines

SPARSE:drivers/media/test-drivers/vidtv/vidtv_pes.c drivers/media/test-drivers/vidtv/vidtv_pes.c:80:54: warning: missing braces around initializer

I assume this refers to the following line, which is the same deal as above.

Quoted 1 line

struct vidtv_pes_optional_pts pts = {0};

And then there's this, which is an actual mistake. I have mostly eliminated these, but this guy has slipped by.

Quoted 3 lines

SPARSE:drivers/media/test-drivers/vidtv/vidtv_s302m.c drivers/media/test-drivers/vidtv/vidtv_s302m.c:430:36: warning:
incorrect type in assignment (different base types)

Just one more thing. This change on vidtv_bridge.c:

Quoted 14 lines

vidtv_bridge_check_demod_lock(struct vidtv_dvb *dvb, u32 n)

dvb->fe[n]->ops.read_status(dvb->fe[n], &status);

(quoted diff) +5 -5
-	return status == (FE_HAS_SIGNAL |
-	FE_HAS_CARRIER |
-	FE_HAS_VITERBI |
-	FE_HAS_SYNC |
-	FE_HAS_LOCK);
+	status = FE_HAS_SIGNAL |
+	FE_HAS_CARRIER |
+	FE_HAS_VITERBI |
+	FE_HAS_SYNC |
+	FE_HAS_LOCK;
Quoted 8 lines
Quoted 3 lines
(quoted diff) +2 -0
+
+	return status;

}

I did not understand that. Why was the boolean expression replaced by an assignment? This was so eventually we could drop packets or simulate some sort of noise in the event that one of these flags was not set, as we've discussed at some point.

Ah, sorry, I remember gcc complained about that (can't remember why).

On a very quick look (it was 2am here when I looked at the code), it sounded to me that you wanted to assign status to some value and return it.

Thanks,
Mauro
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees