Linux userland API discussions
 help / color / mirror / Atom feed
* [PATCH 19/26] [media] frontend: Move legacy API enums/structs to the end
From: Mauro Carvalho Chehab @ 2015-06-08 19:54 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab,
	linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1433792665.git.mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

In order to better organize the header file, move the legacy
API (DVBv3) support to the end, just before the ioctl definitions.

This way, we can use just one #if for all of them.

Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
index 7f829c92dd64..75605a7670a9 100644
--- a/include/uapi/linux/dvb/frontend.h
+++ b/include/uapi/linux/dvb/frontend.h
@@ -216,19 +216,6 @@ enum fe_transmit_mode {
 
 typedef enum fe_transmit_mode fe_transmit_mode_t;
 
-#if defined(__DVB_CORE__) || !defined (__KERNEL__)
-enum fe_bandwidth {
-	BANDWIDTH_8_MHZ,
-	BANDWIDTH_7_MHZ,
-	BANDWIDTH_6_MHZ,
-	BANDWIDTH_AUTO,
-	BANDWIDTH_5_MHZ,
-	BANDWIDTH_10_MHZ,
-	BANDWIDTH_1_712_MHZ,
-};
-
-typedef enum fe_bandwidth fe_bandwidth_t;
-#endif
 
 enum fe_guard_interval {
 	GUARD_INTERVAL_1_32,
@@ -263,51 +250,6 @@ enum fe_interleaving {
 	INTERLEAVING_720,
 };
 
-#if defined(__DVB_CORE__) || !defined (__KERNEL__)
-struct dvb_qpsk_parameters {
-	__u32		symbol_rate;  /* symbol rate in Symbols per second */
-	fe_code_rate_t	fec_inner;    /* forward error correction (see above) */
-};
-
-struct dvb_qam_parameters {
-	__u32		symbol_rate; /* symbol rate in Symbols per second */
-	fe_code_rate_t	fec_inner;   /* forward error correction (see above) */
-	fe_modulation_t	modulation;  /* modulation type (see above) */
-};
-
-struct dvb_vsb_parameters {
-	fe_modulation_t	modulation;  /* modulation type (see above) */
-};
-
-struct dvb_ofdm_parameters {
-	fe_bandwidth_t      bandwidth;
-	fe_code_rate_t      code_rate_HP;  /* high priority stream code rate */
-	fe_code_rate_t      code_rate_LP;  /* low priority stream code rate */
-	fe_modulation_t     constellation; /* modulation type (see above) */
-	fe_transmit_mode_t  transmission_mode;
-	fe_guard_interval_t guard_interval;
-	fe_hierarchy_t      hierarchy_information;
-};
-
-
-struct dvb_frontend_parameters {
-	__u32 frequency;     /* (absolute) frequency in Hz for QAM/OFDM/ATSC */
-			     /* intermediate frequency in kHz for QPSK */
-	fe_spectral_inversion_t inversion;
-	union {
-		struct dvb_qpsk_parameters qpsk;
-		struct dvb_qam_parameters  qam;
-		struct dvb_ofdm_parameters ofdm;
-		struct dvb_vsb_parameters vsb;
-	} u;
-};
-
-struct dvb_frontend_event {
-	fe_status_t status;
-	struct dvb_frontend_parameters parameters;
-};
-#endif
-
 /* S2API Commands */
 #define DTV_UNDEFINED		0
 #define DTV_TUNE		1
@@ -582,6 +524,64 @@ struct dtv_properties {
 	struct dtv_property *props;
 };
 
+#if defined(__DVB_CORE__) || !defined (__KERNEL__)
+
+enum fe_bandwidth {
+	BANDWIDTH_8_MHZ,
+	BANDWIDTH_7_MHZ,
+	BANDWIDTH_6_MHZ,
+	BANDWIDTH_AUTO,
+	BANDWIDTH_5_MHZ,
+	BANDWIDTH_10_MHZ,
+	BANDWIDTH_1_712_MHZ,
+};
+
+typedef enum fe_bandwidth fe_bandwidth_t;
+
+struct dvb_qpsk_parameters {
+	__u32		symbol_rate;  /* symbol rate in Symbols per second */
+	fe_code_rate_t	fec_inner;    /* forward error correction (see above) */
+};
+
+struct dvb_qam_parameters {
+	__u32		symbol_rate; /* symbol rate in Symbols per second */
+	fe_code_rate_t	fec_inner;   /* forward error correction (see above) */
+	fe_modulation_t	modulation;  /* modulation type (see above) */
+};
+
+struct dvb_vsb_parameters {
+	fe_modulation_t	modulation;  /* modulation type (see above) */
+};
+
+struct dvb_ofdm_parameters {
+	fe_bandwidth_t      bandwidth;
+	fe_code_rate_t      code_rate_HP;  /* high priority stream code rate */
+	fe_code_rate_t      code_rate_LP;  /* low priority stream code rate */
+	fe_modulation_t     constellation; /* modulation type (see above) */
+	fe_transmit_mode_t  transmission_mode;
+	fe_guard_interval_t guard_interval;
+	fe_hierarchy_t      hierarchy_information;
+};
+
+
+struct dvb_frontend_parameters {
+	__u32 frequency;     /* (absolute) frequency in Hz for QAM/OFDM/ATSC */
+			     /* intermediate frequency in kHz for QPSK */
+	fe_spectral_inversion_t inversion;
+	union {
+		struct dvb_qpsk_parameters qpsk;
+		struct dvb_qam_parameters  qam;
+		struct dvb_ofdm_parameters ofdm;
+		struct dvb_vsb_parameters vsb;
+	} u;
+};
+
+struct dvb_frontend_event {
+	fe_status_t status;
+	struct dvb_frontend_parameters parameters;
+};
+#endif
+
 #define FE_SET_PROPERTY		   _IOW('o', 82, struct dtv_properties)
 #define FE_GET_PROPERTY		   _IOR('o', 83, struct dtv_properties)
 
-- 
2.4.2

^ permalink raw reply related

* [PATCH 20/26] [media] frontend: move legacy typedefs to the end
From: Mauro Carvalho Chehab @ 2015-06-08 19:54 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab,
	linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1433792665.git.mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

Just userspace need those typedefs. So, put it in the compat part
of the header.

Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
index 75605a7670a9..46c7fd1143a5 100644
--- a/include/uapi/linux/dvb/frontend.h
+++ b/include/uapi/linux/dvb/frontend.h
@@ -35,9 +35,6 @@ enum fe_type {
 	FE_ATSC
 };
 
-typedef enum fe_type fe_type_t;
-
-
 enum fe_caps {
 	FE_IS_STUPID			= 0,
 	FE_CAN_INVERSION_AUTO		= 0x1,
@@ -72,9 +69,6 @@ enum fe_caps {
 	FE_CAN_MUTE_TS			= 0x80000000  /* frontend can stop spurious TS data output */
 };
 
-typedef enum fe_caps fe_caps_t;
-
-
 struct dvb_frontend_info {
 	char       name[128];
 	enum fe_type type;	/* DEPRECATED. Use DTV_ENUM_DELSYS instead */
@@ -99,39 +93,28 @@ struct dvb_diseqc_master_cmd {
 	__u8 msg_len;	/*  valid values are 3...6  */
 };
 
-
 struct dvb_diseqc_slave_reply {
 	__u8 msg [4];	/*  { framing, data [3] } */
 	__u8 msg_len;	/*  valid values are 0...4, 0 means no msg  */
 	int  timeout;	/*  return from ioctl after timeout ms with */
 };			/*  errorcode when no message was received  */
 
-
 enum fe_sec_voltage {
 	SEC_VOLTAGE_13,
 	SEC_VOLTAGE_18,
 	SEC_VOLTAGE_OFF
 };
 
-typedef enum fe_sec_voltage fe_sec_voltage_t;
-
-
 enum fe_sec_tone_mode {
 	SEC_TONE_ON,
 	SEC_TONE_OFF
 };
 
-typedef enum fe_sec_tone_mode fe_sec_tone_mode_t;
-
-
 enum fe_sec_mini_cmd {
 	SEC_MINI_A,
 	SEC_MINI_B
 };
 
-typedef enum fe_sec_mini_cmd fe_sec_mini_cmd_t;
-
-
 /**
  * enum fe_status - enumerates the possible frontend status
  * @FE_HAS_SIGNAL:	found something above the noise level
@@ -143,7 +126,6 @@ typedef enum fe_sec_mini_cmd fe_sec_mini_cmd_t;
  * @FE_REINIT:		frontend was reinitialized, application is recommended
  *			to reset DiSEqC, tone and parameters
  */
-
 enum fe_status {
 	FE_HAS_SIGNAL		= 0x01,
 	FE_HAS_CARRIER		= 0x02,
@@ -154,16 +136,12 @@ enum fe_status {
 	FE_REINIT		= 0x40,
 };
 
-typedef enum fe_status fe_status_t;
-
 enum fe_spectral_inversion {
 	INVERSION_OFF,
 	INVERSION_ON,
 	INVERSION_AUTO
 };
 
-typedef enum fe_spectral_inversion fe_spectral_inversion_t;
-
 enum fe_code_rate {
 	FEC_NONE = 0,
 	FEC_1_2,
@@ -180,9 +158,6 @@ enum fe_code_rate {
 	FEC_2_5,
 };
 
-typedef enum fe_code_rate fe_code_rate_t;
-
-
 enum fe_modulation {
 	QPSK,
 	QAM_16,
@@ -200,8 +175,6 @@ enum fe_modulation {
 	QAM_4_NR,
 };
 
-typedef enum fe_modulation fe_modulation_t;
-
 enum fe_transmit_mode {
 	TRANSMISSION_MODE_2K,
 	TRANSMISSION_MODE_8K,
@@ -214,9 +187,6 @@ enum fe_transmit_mode {
 	TRANSMISSION_MODE_C3780,
 };
 
-typedef enum fe_transmit_mode fe_transmit_mode_t;
-
-
 enum fe_guard_interval {
 	GUARD_INTERVAL_1_32,
 	GUARD_INTERVAL_1_16,
@@ -231,8 +201,6 @@ enum fe_guard_interval {
 	GUARD_INTERVAL_PN945,
 };
 
-typedef enum fe_guard_interval fe_guard_interval_t;
-
 enum fe_hierarchy {
 	HIERARCHY_NONE,
 	HIERARCHY_1,
@@ -241,8 +209,6 @@ enum fe_hierarchy {
 	HIERARCHY_AUTO
 };
 
-typedef enum fe_hierarchy fe_hierarchy_t;
-
 enum fe_interleaving {
 	INTERLEAVING_NONE,
 	INTERLEAVING_AUTO,
@@ -349,8 +315,6 @@ enum fe_pilot {
 	PILOT_AUTO,
 };
 
-typedef enum fe_pilot fe_pilot_t;
-
 enum fe_rolloff {
 	ROLLOFF_35, /* Implied value in DVB-S, default for DVB-S2 */
 	ROLLOFF_20,
@@ -358,8 +322,6 @@ enum fe_rolloff {
 	ROLLOFF_AUTO,
 };
 
-typedef enum fe_rolloff fe_rolloff_t;
-
 enum fe_delivery_system {
 	SYS_UNDEFINED,
 	SYS_DVBC_ANNEX_A,
@@ -382,8 +344,6 @@ enum fe_delivery_system {
 	SYS_DVBC_ANNEX_C,
 };
 
-typedef enum fe_delivery_system fe_delivery_system_t;
-
 /* backward compatibility */
 #define SYS_DVBC_ANNEX_AC	SYS_DVBC_ANNEX_A
 #define SYS_DMBTH SYS_DTMB /* DMB-TH is legacy name, use DTMB instead */
@@ -536,7 +496,23 @@ enum fe_bandwidth {
 	BANDWIDTH_1_712_MHZ,
 };
 
+/* This is needed for legacy userspace support */
+typedef enum fe_sec_voltage fe_sec_voltage_t;
+typedef enum fe_caps fe_caps_t;
+typedef enum fe_type fe_type_t;
+typedef enum fe_sec_tone_mode fe_sec_tone_mode_t;
+typedef enum fe_sec_mini_cmd fe_sec_mini_cmd_t;
+typedef enum fe_status fe_status_t;
+typedef enum fe_spectral_inversion fe_spectral_inversion_t;
+typedef enum fe_code_rate fe_code_rate_t;
+typedef enum fe_modulation fe_modulation_t;
+typedef enum fe_transmit_mode fe_transmit_mode_t;
 typedef enum fe_bandwidth fe_bandwidth_t;
+typedef enum fe_guard_interval fe_guard_interval_t;
+typedef enum fe_hierarchy fe_hierarchy_t;
+typedef enum fe_pilot fe_pilot_t;
+typedef enum fe_rolloff fe_rolloff_t;
+typedef enum fe_delivery_system fe_delivery_system_t;
 
 struct dvb_qpsk_parameters {
 	__u32		symbol_rate;  /* symbol rate in Symbols per second */
@@ -563,7 +539,6 @@ struct dvb_ofdm_parameters {
 	fe_hierarchy_t      hierarchy_information;
 };
 
-
 struct dvb_frontend_parameters {
 	__u32 frequency;     /* (absolute) frequency in Hz for QAM/OFDM/ATSC */
 			     /* intermediate frequency in kHz for QPSK */
@@ -585,7 +560,6 @@ struct dvb_frontend_event {
 #define FE_SET_PROPERTY		   _IOW('o', 82, struct dtv_properties)
 #define FE_GET_PROPERTY		   _IOR('o', 83, struct dtv_properties)
 
-
 /**
  * When set, this flag will disable any zigzagging or other "normal" tuning
  * behaviour. Additionally, there will be no automatic monitoring of the lock
@@ -595,7 +569,6 @@ struct dvb_frontend_event {
  */
 #define FE_TUNE_MODE_ONESHOT 0x01
 
-
 #define FE_GET_INFO		   _IOR('o', 61, struct dvb_frontend_info)
 
 #define FE_DISEQC_RESET_OVERLOAD   _IO('o', 62)
-- 
2.4.2

^ permalink raw reply related

* [PATCH 22/26] [media] frontend: Fix a typo at the comments
From: Mauro Carvalho Chehab @ 2015-06-08 19:54 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab,
	linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1433792665.git.mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

The description of struct dtv_stats has a spmall typo:
	FE_SCALE_DECIBELS instead of FE_SCALE_DECIBEL

Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
index 46c7fd1143a5..0380e62fc8b2 100644
--- a/include/uapi/linux/dvb/frontend.h
+++ b/include/uapi/linux/dvb/frontend.h
@@ -435,13 +435,13 @@ enum fecap_scale_params {
  *
  * In other words, for ISDB, those values should be filled like:
  *	u.st.stat.svalue[0] = global statistics;
- *	u.st.stat.scale[0] = FE_SCALE_DECIBELS;
+ *	u.st.stat.scale[0] = FE_SCALE_DECIBEL;
  *	u.st.stat.value[1] = layer A statistics;
  *	u.st.stat.scale[1] = FE_SCALE_NOT_AVAILABLE (if not available);
  *	u.st.stat.svalue[2] = layer B statistics;
- *	u.st.stat.scale[2] = FE_SCALE_DECIBELS;
+ *	u.st.stat.scale[2] = FE_SCALE_DECIBEL;
  *	u.st.stat.svalue[3] = layer C statistics;
- *	u.st.stat.scale[3] = FE_SCALE_DECIBELS;
+ *	u.st.stat.scale[3] = FE_SCALE_DECIBEL;
  *	u.st.len = 4;
  */
 struct dtv_stats {
-- 
2.4.2

^ permalink raw reply related

* [PATCH 23/26] [media] dvb: frontend.h: improve dvb_frontent_parameters comment
From: Mauro Carvalho Chehab @ 2015-06-08 19:54 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab,
	linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1433792665.git.mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

The comment for struct dvb_frontend_parameters is weird, as it
mixes delivery system name (ATSC) with modulation names
(QPSK, QAM, OFDM).

Use delivery system names there on the frequency comment, as this
is clearer, specially after 2GEN delivery systems.

While here, add comments at the union, to make live easier for ones
that may try to understand the convention used by the legacy API.

Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
index 0380e62fc8b2..e764fd8b7e35 100644
--- a/include/uapi/linux/dvb/frontend.h
+++ b/include/uapi/linux/dvb/frontend.h
@@ -540,14 +540,14 @@ struct dvb_ofdm_parameters {
 };
 
 struct dvb_frontend_parameters {
-	__u32 frequency;     /* (absolute) frequency in Hz for QAM/OFDM/ATSC */
-			     /* intermediate frequency in kHz for QPSK */
+	__u32 frequency;     /* (absolute) frequency in Hz for DVB-C/DVB-T/ATSC */
+			     /* intermediate frequency in kHz for DVB-S */
 	fe_spectral_inversion_t inversion;
 	union {
-		struct dvb_qpsk_parameters qpsk;
-		struct dvb_qam_parameters  qam;
-		struct dvb_ofdm_parameters ofdm;
-		struct dvb_vsb_parameters vsb;
+		struct dvb_qpsk_parameters qpsk;	/* DVB-S */
+		struct dvb_qam_parameters  qam;		/* DVB-C */
+		struct dvb_ofdm_parameters ofdm;	/* DVB-T */
+		struct dvb_vsb_parameters vsb;		/* ATSC */
 	} u;
 };
 
-- 
2.4.2

^ permalink raw reply related

* [PATCH 24/26] [media] dvb: frontend.h: add a note for the deprecated enums/structs
From: Mauro Carvalho Chehab @ 2015-06-08 19:54 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab,
	linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1433792665.git.mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

Let be clear, at the header, about what got deprecated.

Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
index e764fd8b7e35..00a20cd21ee2 100644
--- a/include/uapi/linux/dvb/frontend.h
+++ b/include/uapi/linux/dvb/frontend.h
@@ -486,6 +486,12 @@ struct dtv_properties {
 
 #if defined(__DVB_CORE__) || !defined (__KERNEL__)
 
+/*
+ * DEPRECATED: The DVBv3 ioctls, structs and enums should not be used on
+ * newer programs, as it doesn't support the second generation of digital
+ * TV standards, nor supports newer delivery systems.
+ */
+
 enum fe_bandwidth {
 	BANDWIDTH_8_MHZ,
 	BANDWIDTH_7_MHZ,
-- 
2.4.2

^ permalink raw reply related

* [PATCH 25/26] [media] dvb: dmx.h: don't use anonymous enums
From: Mauro Carvalho Chehab @ 2015-06-08 19:54 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab,
	linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1433792665.git.mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

There are several anonymous enums here, used via a typedef.

Well, we don't like typedefs on Kernel, so let's de-anonimize
those enums. Then, latter, we may be able to get rid of the
typedefs, at least from Kernelspace.

Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

diff --git a/include/uapi/linux/dvb/dmx.h b/include/uapi/linux/dvb/dmx.h
index b4fb650d9d4f..ece3661a3cac 100644
--- a/include/uapi/linux/dvb/dmx.h
+++ b/include/uapi/linux/dvb/dmx.h
@@ -32,7 +32,7 @@
 
 #define DMX_FILTER_SIZE 16
 
-typedef enum
+typedef enum dmx_output
 {
 	DMX_OUT_DECODER, /* Streaming directly to decoder. */
 	DMX_OUT_TAP,     /* Output going to a memory buffer */
@@ -44,7 +44,7 @@ typedef enum
 } dmx_output_t;
 
 
-typedef enum
+typedef enum dmx_input
 {
 	DMX_IN_FRONTEND, /* Input from a front-end device.  */
 	DMX_IN_DVR       /* Input from the logical DVR device.  */
@@ -122,7 +122,7 @@ typedef struct dmx_caps {
 	int num_decoders;
 } dmx_caps_t;
 
-typedef enum {
+typedef enum dmx_source {
 	DMX_SOURCE_FRONT0 = 0,
 	DMX_SOURCE_FRONT1,
 	DMX_SOURCE_FRONT2,
-- 
2.4.2

^ permalink raw reply related

* [PATCH 26/26] [media] DocBook: Change format for enum dmx_output documentation
From: Mauro Carvalho Chehab @ 2015-06-08 19:54 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Jonathan Corbet,
	linux-doc, linux-api
In-Reply-To: <cover.1433792665.git.mchehab@osg.samsung.com>

Use a table for the Demux output. No new information added
here. They were all merged inside the table.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/Documentation/DocBook/media/Makefile b/Documentation/DocBook/media/Makefile
index e7d44a0b00d6..23996f88cd58 100644
--- a/Documentation/DocBook/media/Makefile
+++ b/Documentation/DocBook/media/Makefile
@@ -88,12 +88,8 @@ ENUMS = \
 
 ENUM_DEFS = \
 	$(shell perl -e 'open IN,"cat @ARGV| cpp -fpreprocessed |"; while (<IN>) { if ($$enum) {print "$$1\n" if (/\s*([A-Z]\S+)\b/); } $$enum = 0 if ($$enum && /^\}/); $$enum = 1 if(/^\s*enum\s/); }; close IN;' \
-		$(srctree)/include/uapi/linux/dvb/audio.h \
-		$(srctree)/include/uapi/linux/dvb/ca.h \
 		$(srctree)/include/uapi/linux/dvb/dmx.h \
-		$(srctree)/include/uapi/linux/dvb/frontend.h \
-		$(srctree)/include/uapi/linux/dvb/net.h \
-		$(srctree)/include/uapi/linux/dvb/video.h)
+		$(srctree)/include/uapi/linux/dvb/frontend.h)
 
 STRUCTS = \
 	$(shell perl -ne 'print "$$1 " if /^struct\s+([^\s]+)\s+/' $(srctree)/include/uapi/linux/videodev2.h) \
@@ -251,9 +247,14 @@ $(MEDIA_OBJ_DIR)/dmx.h.xml: $(srctree)/include/uapi/linux/dvb/dmx.h $(MEDIA_OBJ_
 	@(					\
 	echo "<programlisting>") > $@
 	@(					\
+	for ident in $(ENUM_DEFS) ; do		\
+	  entity=`echo $$ident | tr _ -` ;	\
+	  r="$$r s/([^\w\-])$$ident([^\w\-])/\1\&$$entity\;\2/g;";\
+	done;					\
 	expand --tabs=8 < $< |			\
 	  sed $(ESCAPE) $(DVB_DOCUMENTED) |	\
-	  sed 's/i\.e\./&ie;/') >> $@
+	  sed 's/i\.e\./&ie;/' |		\
+	  perl -ne "$$r print $$_;") >> $@
 	@(					\
 	echo "</programlisting>") >> $@
 
diff --git a/Documentation/DocBook/media/dvb/demux.xml b/Documentation/DocBook/media/dvb/demux.xml
index 11a831d58643..34f2fb1cd601 100644
--- a/Documentation/DocBook/media/dvb/demux.xml
+++ b/Documentation/DocBook/media/dvb/demux.xml
@@ -8,26 +8,43 @@ accessed by including <constant>linux/dvb/dmx.h</constant> in your application.
 <title>Demux Data Types</title>
 
 <section id="dmx-output-t">
-<title>dmx_output_t</title>
-<programlisting>
-typedef enum
-{
-	DMX_OUT_DECODER, /&#x22C6; Streaming directly to decoder. &#x22C6;/
-	DMX_OUT_TAP,     /&#x22C6; Output going to a memory buffer &#x22C6;/
-			 /&#x22C6; (to be retrieved via the read command).&#x22C6;/
-	DMX_OUT_TS_TAP,  /&#x22C6; Output multiplexed into a new TS  &#x22C6;/
-			 /&#x22C6; (to be retrieved by reading from the &#x22C6;/
-			 /&#x22C6; logical DVR device).                 &#x22C6;/
-	DMX_OUT_TSDEMUX_TAP /&#x22C6; Like TS_TAP but retrieved from the DMX device &#x22C6;/
-} dmx_output_t;
-</programlisting>
-<para><constant>DMX_OUT_TAP</constant> delivers the stream output to the demux device on which the ioctl is
-called.
-</para>
-<para><constant>DMX_OUT_TS_TAP</constant> routes output to the logical DVR device <constant>/dev/dvb/adapter?/dvr?</constant>,
-which delivers a TS multiplexed from all filters for which <constant>DMX_OUT_TS_TAP</constant> was
-specified.
-</para>
+<title>Output for the demux</title>
+
+<table pgwide="1" frame="none" id="dmx-output">
+    <title>enum dmx_output</title>
+    <tgroup cols="2">
+	&cs-def;
+	<thead>
+	<row>
+	    <entry>ID</entry>
+	    <entry>Description</entry>
+	</row>
+	</thead>
+	<tbody valign="top">
+	<row>
+		<entry align="char" id="DMX-OUT-DECODER">DMX_OUT_DECODER</entry>
+		<entry>Streaming directly to decoder.</entry>
+	</row><row>
+		<entry align="char" id="DMX-OUT-TAP">DMX_OUT_TAP</entry>
+		<entry>Output going to a memory buffer (to be retrieved via the
+		    read command). Delivers the stream output to the demux
+		    device on which the ioctl is called.</entry>
+	</row><row>
+		<entry align="char" id="DMX-OUT-TS-TAP">DMX_OUT_TS_TAP</entry>
+		<entry>Output multiplexed into a new TS (to be retrieved by
+		    reading from the logical DVR device). Routes output to the
+		    logical DVR device <constant>/dev/dvb/adapter?/dvr?</constant>,
+		    which delivers a TS multiplexed from all filters for which
+		    <constant>DMX_OUT_TS_TAP</constant> was specified.</entry>
+	</row><row>
+		<entry align="char" id="DMX-OUT-TSDEMUX-TAP">DMX_OUT_TSDEMUX_TAP</entry>
+		<entry>Like &DMX-OUT-TS-TAP; but retrieved from the DMX
+		    device.</entry>
+	</row>
+        </tbody>
+    </tgroup>
+</table>
+
 </section>
 
 <section id="dmx-input-t">
diff --git a/include/uapi/linux/dvb/dmx.h b/include/uapi/linux/dvb/dmx.h
index ece3661a3cac..427e4899ed69 100644
--- a/include/uapi/linux/dvb/dmx.h
+++ b/include/uapi/linux/dvb/dmx.h
@@ -32,7 +32,7 @@
 
 #define DMX_FILTER_SIZE 16
 
-typedef enum dmx_output
+enum dmx_output
 {
 	DMX_OUT_DECODER, /* Streaming directly to decoder. */
 	DMX_OUT_TAP,     /* Output going to a memory buffer */
@@ -41,8 +41,9 @@ typedef enum dmx_output
 			 /* (to be retrieved by reading from the */
 			 /* logical DVR device).                 */
 	DMX_OUT_TSDEMUX_TAP /* Like TS_TAP but retrieved from the DMX device */
-} dmx_output_t;
+};
 
+typedef enum dmx_output dmx_output_t;
 
 typedef enum dmx_input
 {
@@ -139,7 +140,6 @@ struct dmx_stc {
 	__u64 stc;		/* output: stc in 'base'*90 kHz units */
 };
 
-
 #define DMX_START                _IO('o', 41)
 #define DMX_STOP                 _IO('o', 42)
 #define DMX_SET_FILTER           _IOW('o', 43, struct dmx_sct_filter_params)
-- 
2.4.2


^ permalink raw reply related

* Re: [PATCH 18/26] [media] dvb: Get rid of typedev usage for enums
From: Joe Perches @ 2015-06-08 20:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Antti Palosaari,
	Jemma Denson, Patrick Boettcher, Michael Krufky,
	Malcolm Priestley, Stefan Richter, Greg Kroah-Hartman, Shuah Khan,
	Akihiro Tsukada, Martin Kepplinger, Richard Vollkommer,
	Hans Verkuil, Geert Uytterhoeven, Julia Lawall, Himangi Saraogi,
	Kees Cook, Heinrich Schuchardt, Cheolhyun Park, Markus Elfring,
	Benoit
In-Reply-To: <336c59e13064f2fa872a1682268f1995deb18fa5.1433792665.git.mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>

On Mon, 2015-06-08 at 16:54 -0300, Mauro Carvalho Chehab wrote:
> The DVB API was originally defined using typedefs. This is against
> Kernel CodingStyle, and there's no good usage here. While we can't
> remove its usage on userspace, we can avoid its usage in Kernelspace.
> 
> So, let's do it.
> 
> This patch was generated by this shell script:
> 
> 	for j in $(grep typedef include/uapi/linux/dvb/frontend.h |cut -d' ' -f 3); do for i in $(find drivers/media -name '*.[ch]' -type f) $(find drivers/staging/media -name '*.[ch]' -type f); do sed "s,${j}_t,enum $j," <$i >a && mv a $i; done; done

Seems sensible, thanks.

> While here, make CodingStyle fixes on the affected lines.

Trivial note examples:

> diff --git a/drivers/media/common/b2c2/flexcop-fe-tuner.c b/drivers/media/common/b2c2/flexcop-fe-tuner.c
[]
> @@ -39,7 +39,8 @@ static int flexcop_fe_request_firmware(struct dvb_frontend *fe,
>  
>  /* lnb control */
>  #if FE_SUPPORTED(MT312) || FE_SUPPORTED(STV0299)
> -static int flexcop_set_voltage(struct dvb_frontend *fe, fe_sec_voltage_t voltage)
> +static int flexcop_set_voltage(struct dvb_frontend *fe,
> +			       enum fe_sec_voltage voltage)

Aligned to open paren.

> @@ -157,7 +158,7 @@ static int flexcop_diseqc_send_master_cmd(struct dvb_frontend *fe,
>  }
>  
>  static int flexcop_diseqc_send_burst(struct dvb_frontend *fe,
> -	fe_sec_mini_cmd_t minicmd)
> +	enum fe_sec_mini_cmd minicmd)

Not aligned to open paren.

Why some and not all?

> diff --git a/drivers/media/common/siano/smsdvb.h b/drivers/media/common/siano/smsdvb.h
[]
> @@ -40,7 +40,7 @@ struct smsdvb_client_t {
>  	struct dmxdev           dmxdev;
>  	struct dvb_frontend     frontend;
>  
> -	fe_status_t             fe_status;
> +	enum fe_status             fe_status;

Maybe realign these too.

^ permalink raw reply

* Re: [RFC PATCH 06/18] signal/kthread: Initial implementation of kthread signal handling
From: Oleg Nesterov @ 2015-06-08 21:13 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Andrew Morton, Tejun Heo, Ingo Molnar, Peter Zijlstra,
	Richard Weinberger, Steven Rostedt, David Woodhouse,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Trond Myklebust,
	Anna Schumaker, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Chris Mason,
	Paul E. McKenney, Thomas Gleixner, Linus Torvalds, Jiri Kosina,
	Borislav Petkov, Michal Hocko,
	live-patching-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150608135107.GB3135-KsEp0d+Q8qECVLCxKZUutA@public.gmane.org>

Let me first repeat that I agree that everything is subjective ;)

On 06/08, Petr Mladek wrote:
>
> To be honest, this patch set does _not_ make any big change.

But to me it does because (again, imo) it adds the a) unnecessary
and b) wrong interface.

But yes, yes, I agree that most (all?) of kthread/signal (ab)users
need cleanups. And fixes.

> I think that we should make it independent on the iterant kthread API.

Yes! please. Then we can discuss this again and perhaps reconsider
this API.

So I am going to ignore some parts of your email. I am sleeping,
please let me know if I missed something important ;)

> Well, note that allow_signal() sets some "crazy" value (2) for the
> signal handler. IMHO, we should check for these values and handle
> them reasonably even in kthreads. It will make the code more secure.

Not sure I understand. The crazy "2" value just means that kthread
wants to recieve and dequeue this signal. I agree with the good name
for this hard-coded number in advance.

> > > +
> > > +		/* Run the custom handler if any */
> > > +		if (ka->sa.kthread_sa_handler != KTHREAD_SIG_DFL) {
> > > +			ksig.ka = *ka;
> > > +
> > > +			if (ka->sa.sa_flags & SA_ONESHOT)
> > > +				ka->sa.kthread_sa_handler = KTHREAD_SIG_DFL;
> > > +
> > > +			spin_unlock_irqrestore(&sighand->siglock, flags);
> > > +			/* could run directly for kthreads */
> > > +			ksig.ka.sa.kthread_sa_handler(signr);
> > > +			freezable_cond_resched();
> > > +			goto relock;
> >
> > Well. But for what? A simple "switch (signr)" after kthread_dequeue_signal()
> > can do the same. Or, speaking of kthread_iterant_fn() it can even dequeue the
> > signal and pass it to kti->whatever(signr).
>
> I wanted to make it independent on the iterant API. Also if you want to
> handle more signals, you need even more code, e.g. the cycle,
> cond_resched(). So, I think that some generic helper is useful.

I do not. Contrary, I think this needs more code in the likely case.
Anyway, this API won't have too many users, so I don't even this this
is that important.

> > > +		if (sig_kernel_stop(signr)) {
> > > +			__set_current_state(TASK_STOPPED);
> > > +			spin_unlock_irqrestore(&sighand->siglock, flags);
> > > +			/* Don't run again until woken by SIGCONT or SIGKILL */
> > > +			freezable_schedule();
> > > +			goto relock;
> >
> > Yes this avoids the race with SIGCONT. But as I said we can add another
> > trivial helper which checks JOBCTL_STOP_DEQUEUED. So a kthread can do
> > this itself.
>
> Hmm, the helper would have a strange semantic. You need to take
> sighand->siglock, dequeue the signal (SIGSTOP), and call
> __set_current_state(TASK_STOPPED) before you release the lock.
> But what would happen if the dequeued signal is _not_ SIGSTOP?

Perhaps I missed your point, but no. If you want to handle SIGSTOP
you can do

	signr = kthread_signal_dequeue();
	switch (signr) {
	case SIGSTOP:
		something_else();
		kthread_do_signal_stop();
	...
	}


> I think that we should support only the standard handling of
> SIGSTOP. It is closely related with SIGCONT.

Agreed. If kthread wants to actually sleep in TASK_STOPPED state then
it should know about SIGCONT.

> > To me, SIG_DFL behaviour just makes makes no sense when it comes to
> > kthreads. I do not even think this can simplify the code. Unlike user-
> > space task, kthread can happily dequeue SIGSTOP, so why should we mimic
> > the userspace SIG_DFL logic.
>
> Maybe, we should handle only SIGSTOP

So far I even disagree with SIGSTOP "default" semantics. I simply see
no value.

Oleg.

^ permalink raw reply

* Re: [GIT PULL] Documentation/features: Add kernel feature descriptions and arch support status files under Documentation/features/
From: Jonathan Corbet @ 2015-06-08 23:08 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andrew Morton, Josh Triplett, Borislav Petkov, Peter Zijlstra,
	Andy Lutomirski, Ingo Molnar, H. Peter Anvin, Thomas Gleixner,
	Linus Torvalds, linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A,
	linux-arch-u79uwXL29TY76Z2rM5mHXA, Peter Zijlstra
In-Reply-To: <20150603110355.GA12079-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On Wed, 3 Jun 2015 13:03:55 +0200
Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> A week late but I managed to complete it all and moved it into Git, for better 
> structure and easy pulling.

Pulled into the docs tree - thanks!

jon

^ permalink raw reply

* Re: [RFC PATCH 00/18] kthreads/signal: Safer kthread API and signal handling
From: Tejun Heo @ 2015-06-09  6:10 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
	Richard Weinberger, Steven Rostedt, David Woodhouse,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Trond Myklebust,
	Anna Schumaker, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Chris Mason,
	Paul E. McKenney, Thomas Gleixner, Linus Torvalds, Jiri Kosina,
	Borislav Petkov, Michal Hocko,
	live-patching-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1433516477-5153-1-git-send-email-pmladek-AlSwsSmVLrQ@public.gmane.org>

Hello,

On Fri, Jun 05, 2015 at 05:00:59PM +0200, Petr Mladek wrote:
> Workqueue
...
> But there are many kthreads that need to cycle many times
> until some work is finished, e.g. khugepaged, virtio_balloon,
> jffs2_garbage_collect_thread. They would need to queue the
> work item repeatedly from the same work item or between
> more work items. It would be a strange semantic.

Hmm... took a look at balloon and jffs2_garbage_collect_thread and I'm
not not sure about this part of argument.  What's wrong with doing a
single round and requeueing if more rounds are necessary?  There are
quite a few which already behave that way.  It sure adds queueing /
dispatch overhead but I'm skeptical that's gonna be anything
noticeable in vast majority of cases.

> Work queues allow to share the same kthread between more users.
> It helps to reduce the number of running kthreads. It is especially
> useful if you would need a kthread for each CPU.

Unbound workqueues also do NUMA node affinities automatically which
usually is a pretty big win for jobs which process lots of data.
Managing concurrency level for these cases is still cumbersome but
there have been some ideas making that part mostly automatic too.

> But this might also be a disadvantage. Just look into the output
> of the command "ps" and see the many [kworker*] processes. One
> might see this a black hole. If a kworker makes the system busy,
> it is less obvious what the problem is in compare with the old
> "simple" and dedicated kthreads.
>
> Yes, we could add some debugging tools for work queues but
> it would be another non-standard thing that developers and
> system administrators would need to understand.

Yeah, that's an almost inherent drawback of pooling anything.  It'd be
nice to improve visibility into what workers are doing.  sysrq-t has
recently been extended to help debugging but it'd nice to have
visibility into who's consuming how much.

> Another thing is that work queues have their own scheduler. If we
> move even more tasks there it might need even more love. Anyway,
> the extra scheduler adds another level of complexity when
> debugging problems.

That's only true for the concurrency managed percpu workqueues which
shouldn't be used for anything CPU intensive anyway.  I don't see that
getting much more sophiscated than now.  For unbound workqueues, once
a work item starts executing, the worker thread is under the control
of the scheduler.

> kthread_worker
> 
> 
> kthread_worker is similar to workqueues in many ways. You need to
> 
>   + define work functions
>   + define and initialize work structs
>   + queue work items (structs pointing to the functions and data)
> 
> We could repeat the paragraphs about splitting the work
> and sharing the kthread between more users here.
> 
> Well, the kthread_worker implementation is much simpler than
> the one for workqueues. It is more similar to a simple
> kthread. Especially, it uses the system scheduler.
> But it is still more complex that the simple kthread.
> 
> One interesting thing is that kthread_workers add internal work
> items into the queue. They typically use a completion. An example
> is the flush work. see flush_kthread_work(). It is a nice trick
> but you need to be careful. For example, if you would want to
> terminate the kthread, you might want to remove some work item
> from the queue, especially if you need to break a work item that
> is called in a cycle (queues itself). The question is what to do
> with the internal tasks. If you keep them, they might wake up
> sleepers when the work was not really completed. If you remove
> them, the counter part might sleep forever.

You can pretty much align this part of interface with regular
workqueues which has cancel_work_sync().  The only reason it doesn't
have that is because nobody needed it yet.

That said, there sure are cases where dedicated kthreads are necessary
and kthread_worker was added to help those cases to be more
structured.  I think it's nice to keep the API aligned with workqueue
so that users can easily be converted back and forth but if something
which is more aligned to the current kthread usages eases replacing
the original kthread API completely, I'm all for that.

Also, if this one takes off, I don't think we have any need for
kthread_worker.  It should be easy to replace the few kthread_worker
usages to the new API.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC PATCH 00/18] kthreads/signal: Safer kthread API and signal handling
From: Tejun Heo @ 2015-06-09  6:14 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Petr Mladek, Andrew Morton, Oleg Nesterov, Ingo Molnar,
	Richard Weinberger, Steven Rostedt, David Woodhouse,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Trond Myklebust,
	Anna Schumaker, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Chris Mason,
	Paul E. McKenney, Thomas Gleixner, Linus Torvalds, Jiri Kosina,
	Borislav Petkov, Michal Hocko,
	live-patching-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150605162216.GK19282-ndre7Fmf5hadTX5a5knrm8zTDFooKrT+cvkQGrU6aU0@public.gmane.org>

Hey, Peter.

On Fri, Jun 05, 2015 at 06:22:16PM +0200, Peter Zijlstra wrote:
> There's a lot more problems with workqueues:
> 
>  - they're not regular tasks and all the task controls don't work on
>    them. This means all things scheduler, like cpu-affinity, nice, and
>    RT/deadline scheduling policies. Instead there is some half baked
>    secondary interface for some of these.

Because there's a pool of them and the workers come and go
dynamically.  There's no way around it.  The attributes just have to
be per-pool.

>    But this also very much includes things like cgroups, which brings me
>    to the second point.
>
>  - its oblivious to cgroups (as it is to RT priority for example) both
>    leading to priority inversion. A work enqueued from a deep/limited
>    cgroup does not inherit the task's cgroup. Instead this work is ran
>    from the root cgroup.
> 
>    This breaks cgroup isolation, more significantly so when a large part
>    of the actual work is done from workqueues (as some workloads end up
>    being). Instead of being able to control the work, it all ends up in
>    the root cgroup outside of control.

cgroup support will surely be added but I'm not sure we can or should
do inheritance automatically.  Using a different API doesn't solve the
problem automatically either.  A lot of kthreads are shared
system-wide after all.  We'll need an abstraction layer to deal with
that no matter where we do it.

Thanks.

-- 
tejun

^ permalink raw reply

* Re: [RFC PATCH 02/18] kthread: Add API for iterant kthreads
From: Tejun Heo @ 2015-06-09  6:23 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
	Richard Weinberger, Steven Rostedt, David Woodhouse,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Trond Myklebust,
	Anna Schumaker, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Chris Mason,
	Paul E. McKenney, Thomas Gleixner, Linus Torvalds, Jiri Kosina,
	Borislav Petkov, Michal Hocko,
	live-patching-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1433516477-5153-3-git-send-email-pmladek-AlSwsSmVLrQ@public.gmane.org>

Hello, Petr.

On Fri, Jun 05, 2015 at 05:01:01PM +0200, Petr Mladek wrote:
> +static int kthread_iterant_fn(void *kti_ptr)
> +{
> +	struct kthread_iterant *kti = kti_ptr;
> +	void *data = kti->data;
> +
> +	if (kti->init)
> +		kti->init(data);
> +
> +	do {
> +		if (kti->func)
> +			kti->func(data);

Is supporting kthread_iterant w/o the body function intentional?  If
so, did you have anything specific on mind for it?  I don't think it
matters either way.  Just curious how this came to be.

Thanks.

-- 
tejun

^ permalink raw reply

* Re: [PATCH v5 02/21] libnvdimm, nfit: initial libnvdimm infrastructure and NFIT support
From: hch-jcswGhMUV9g @ 2015-06-09  6:33 UTC (permalink / raw)
  To: Williams, Dan J
  Cc: hch-jcswGhMUV9g@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	neilb-l3A5Bk7waGM@public.gmane.org,
	jmoyer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
	linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org,
	sfr-3FnU+UHB4dNDw9hX6IcOSA@public.gmane.org,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
	rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1433359473.21035.27.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

On Wed, Jun 03, 2015 at 07:24:34PM +0000, Williams, Dan J wrote:
> > > +static inline struct acpi_nfit_memory_map *__to_nfit_memdev(struct nfit_mem *nfit_mem)
> > 
> > This line is over 80 characters.
> 
> I generally don't see the point of fixing up occasional small incursions
> over 80 characters if it makes the code more amenable to grep.

Just use shorter type names or add a line break before the symbol
names.  Too long lines are highly annoying.

^ permalink raw reply

* Re: [PATCH v5 05/21] libnvdimm: control (ioctl) messages for libnvdimm bus and dimm devices
From: Christoph Hellwig @ 2015-06-09  6:34 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, axboe-tSWWG44O7X1aa/9Udqfwiw,
	sfr-3FnU+UHB4dNDw9hX6IcOSA, rafael-DgEjT+Ai2ygdnm+yROfE0A,
	neilb-l3A5Bk7waGM, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, mingo-DgEjT+Ai2ygdnm+yROfE0A,
	linux-acpi-u79uwXL29TY76Z2rM5mHXA, jmoyer-H+wXaHxf7aLQT0dZR+AlfA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, hch-jcswGhMUV9g
In-Reply-To: <20150602001435.4506.42614.stgit-p8uTFz9XbKj2zm6wflaqv1nYeNYlB/vhral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

On Mon, Jun 01, 2015 at 08:14:35PM -0400, Dan Williams wrote:
> Most discovery/configuration of the libnvdimm-subsystem is done via
> sysfs attributes.  However, some libnvdimm buses, particularly the
> ACPI.NFIT bus, define a small set of messages that can be passed to the
> platform.  For convenience we derive the initial libnvdimm-ioctl command
> formats directly from the NFIT DSM Interface Example formats.

Why not merge these into the extensive sysfs interface?

^ permalink raw reply

* Re: [PATCH v5 09/21] libnvdimm, nd_pmem: add libnvdimm support to the pmem driver
From: hch-jcswGhMUV9g @ 2015-06-09  6:36 UTC (permalink / raw)
  To: Williams, Dan J
  Cc: hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	linux-nvdimm-y27Ovi1pjclAfugRpC6u6w@public.gmane.org,
	neilb-l3A5Bk7waGM@public.gmane.org,
	linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	hch-jcswGhMUV9g@public.gmane.org,
	axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org,
	sfr-3FnU+UHB4dNDw9hX6IcOSA@public.gmane.org,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
	rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1433359894.21035.33.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

On Wed, Jun 03, 2015 at 07:31:38PM +0000, Williams, Dan J wrote:
> I like move-modify patches because they make the reason for the move
> clearer and revertible.  Consider a general case where we later decide
> the reason for a move was invalid.  Reverting the reason also reverts
> the file move, that doesn't automatically happen when it is split to
> another commit.

BS.  A move is never fundamentally related to the change.  Your
series would work perfectly fine without the move for example.

^ permalink raw reply

* Re: [PATCH v5 12/21] libnvdimm: namespace indices: read and validate
From: Christoph Hellwig @ 2015-06-09  6:39 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, axboe-tSWWG44O7X1aa/9Udqfwiw,
	sfr-3FnU+UHB4dNDw9hX6IcOSA, rafael-DgEjT+Ai2ygdnm+yROfE0A,
	neilb-l3A5Bk7waGM, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, mingo-DgEjT+Ai2ygdnm+yROfE0A,
	linux-acpi-u79uwXL29TY76Z2rM5mHXA, jmoyer-H+wXaHxf7aLQT0dZR+AlfA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, hch-jcswGhMUV9g
In-Reply-To: <20150602001514.4506.13974.stgit-p8uTFz9XbKj2zm6wflaqv1nYeNYlB/vhral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

> +#include <asm-generic/io-64-nonatomic-lo-hi.h>
> +
> +#ifndef __io_virt
> +#define __io_virt(x) ((void __force *) (x))
> +#endif

NAK. driver code mus never include asm-generic headers directly,
and __io_virt isn't something that's up to a driver to redefine either.

I think we really need the memremap series in first before trying to
do the things done in this patch.

^ permalink raw reply

* Re: [PATCH v5 17/21] libnvdimm: infrastructure for btt devices
From: Christoph Hellwig @ 2015-06-09  6:42 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm, axboe, sfr, rafael, neilb, gregkh, linux-kernel,
	mingo, linux-acpi, jmoyer, linux-api, akpm, hch
In-Reply-To: <20150602001541.4506.90125.stgit@dwillia2-desk3.amr.corp.intel.com>

On Mon, Jun 01, 2015 at 08:15:41PM -0400, Dan Williams wrote:
> Block devices from an nd bus, in addition to accepting "struct bio"
> based requests, also have the capability to perform byte-aligned
> accesses.  By default only the bio/block interface is used.  However, if
> another driver can make effective use of the byte-aligned capability it
> can claim/disable the block interface and use the byte-aligned "nd_io"
> interface.
> 
> The BTT driver is the initial first consumer of this mechanism to allow
> layering atomic sector update guarantees on top of nd_io capable
> libnvdimm-block-devices, or their partitions.

As mentioned during the last time of the review the layering here
is complete broken.  If you expose additional capabilities from a block
device do it at the block device level.  That is enhance the rw_page
callback to allo byte sized access, add a capability flag on the queue,
etc.

^ permalink raw reply

* Re: [PATCH v5 18/21] nd_btt: atomic sector updates
From: Christoph Hellwig @ 2015-06-09  6:44 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, axboe-tSWWG44O7X1aa/9Udqfwiw,
	sfr-3FnU+UHB4dNDw9hX6IcOSA, rafael-DgEjT+Ai2ygdnm+yROfE0A,
	neilb-l3A5Bk7waGM, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, mingo-DgEjT+Ai2ygdnm+yROfE0A,
	linux-acpi-u79uwXL29TY76Z2rM5mHXA, jmoyer-H+wXaHxf7aLQT0dZR+AlfA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, hch-jcswGhMUV9g
In-Reply-To: <20150602001546.4506.15713.stgit-p8uTFz9XbKj2zm6wflaqv1nYeNYlB/vhral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

I really want to see a good explanation why this is not a blk-mq driver
given that it does fairly substantial work and has synchronization
in its make_request function.

^ permalink raw reply

* Re: [PATCH v5 20/21] tools/testing/nvdimm: manufactured NFITs for interface development
From: Christoph Hellwig @ 2015-06-09  6:48 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, axboe-tSWWG44O7X1aa/9Udqfwiw,
	sfr-3FnU+UHB4dNDw9hX6IcOSA, rafael-DgEjT+Ai2ygdnm+yROfE0A,
	neilb-l3A5Bk7waGM, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, mingo-DgEjT+Ai2ygdnm+yROfE0A,
	linux-acpi-u79uwXL29TY76Z2rM5mHXA, jmoyer-H+wXaHxf7aLQT0dZR+AlfA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, hch-jcswGhMUV9g
In-Reply-To: <20150602001557.4506.57136.stgit-p8uTFz9XbKj2zm6wflaqv1nYeNYlB/vhral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

The description fails to mention the hacks how this is archived.  I don't
think the include hackery and otherwise needed exports are acceptable
if with an explanation, though.

Really - get your fake hardware into qemu right now and don't bother
with trying to hack the subsystem like this.

^ permalink raw reply

* Re: [PATCH v5 05/21] libnvdimm: control (ioctl) messages for libnvdimm bus and dimm devices
From: Dan Williams @ 2015-06-09  6:57 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org, Jens Axboe,
	Stephen Rothwell, Rafael J. Wysocki, Neil Brown, Greg KH,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Ingo Molnar,
	Linux ACPI, jmoyer, linux-api-u79uwXL29TY76Z2rM5mHXA,
	Andrew Morton
In-Reply-To: <20150609063418.GB9804-jcswGhMUV9g@public.gmane.org>

On Mon, Jun 8, 2015 at 11:34 PM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> On Mon, Jun 01, 2015 at 08:14:35PM -0400, Dan Williams wrote:
>> Most discovery/configuration of the libnvdimm-subsystem is done via
>> sysfs attributes.  However, some libnvdimm buses, particularly the
>> ACPI.NFIT bus, define a small set of messages that can be passed to the
>> platform.  For convenience we derive the initial libnvdimm-ioctl command
>> formats directly from the NFIT DSM Interface Example formats.
>
> Why not merge these into the extensive sysfs interface?

Data payload size for one, these commands transfer more than a page
worth of data at a time.

Even if we killed the ioctl interface to userspace we still need all
the ugly data marshaling code in the kernel to craft properly
formatted ACPI _DSM messages.  I try to share as much common
infrastructure from the ACPI _DSM implementation to the ioctl
interface (nd_cmd_dimm_desc() + nd_cmd_bus_desc()).

^ permalink raw reply

* Re: [RFC PATCH 06/18] signal/kthread: Initial implementation of kthread signal handling
From: Tejun Heo @ 2015-06-09  7:10 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
	Richard Weinberger, Steven Rostedt, David Woodhouse,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Trond Myklebust,
	Anna Schumaker, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Chris Mason,
	Paul E. McKenney, Thomas Gleixner, Linus Torvalds, Jiri Kosina,
	Borislav Petkov, Michal Hocko,
	live-patching-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1433516477-5153-7-git-send-email-pmladek-AlSwsSmVLrQ@public.gmane.org>

Hello, Petr.

On Fri, Jun 05, 2015 at 05:01:05PM +0200, Petr Mladek wrote:
> Several kthreads already handle signals some way. They do so
> in different ways, search for allow_signal().
> 
> This patch allows to handle the signals in a more uniform way using
> kthread_do_signal().
> 
> The main question is how much it should follow POSIX and the signal
> handling of user space processes. On one hand, we want to be as close
> as possible. On the other hand, we need to be very careful. All signals
> were ignored until now, except for the few kthreads that somehow
> handled them.
> 
> POSIX behavior might cause some surprises and crazy bug reports.
> For example, imagine a home user stopping or killing kswapd because
> it makes the system too busy. It is like shooting in its own leg.
> 
> Also the signals are already used a strange way. For example, klockd
> drops all locks when it gets SIGKILL. This was added before initial
> git commit, so the archaeology was not easy. The most likely explanation
> is that it is used during the system shutdown. It would make sense
> to drop all locks when user space processes are killed and before
> filesystems get unmounted.
> 
> Now, the patch does the following compromise:
> 
>   + defines default actions for SIGSTOP, SIGCONT, and fatal signals
>   + custom actions can be defined by kthread_sigaction()
>   + all signals are still ignored by default
> 
> Well, fatal signals do not terminate the kthread immediately. They just
> set a flag to terminate the kthread, flush all other pending signals,
> and return to the main kthread cycle. The kthread is supposed to check
> the flag, leave the main cycle, do some cleanup actions when necessary
> and return from the kthread.
> 
> Note that kthreads are running in the kernel address space. It makes
> the life much easier. The signal handler can be called directly and
> we do not need any arch-specific code to process it in the userspace.
> Also we do not care about ptrace.
> 
> Finally, kthread_do_signal() is called on a safe place in the main
> iterant kthread cycle. Then we will not need any special code for
> signals when using this kthread API.
> 
> This change does not affect any existing kthread. The plan is to
> use them only in safe kthreads that can be converted into
> the iterant kthread API.

While I agree that it'd be great to consolidate the existing kthread
signal users, I feel quite uncomfortable with doing full-fledged
signal handling for kthreads which try to mimic the userland behavior.
Most of the default behaviors don't make sense for kthreads and it'd
be awful to have different kthreads interpreting arbitrary signals for
differing custom behaviors, which often happens when there's no
default.

While we do have several kthread signal users, they aren't too many
and majority of them use it to allow userland to tell it to shutdown
and there seem to be a couple which use HUP/USR1 to cancel whatever
it's processing / waiting on at the moment.  Hmmm... jffs uses
STOP/CONT too.

I don't know how this should be done but let's please try to

1. Encourage uniform behaviors across the signals.

2. Ultimately discourage usage of signals on kthreads as this closely
   ties implementation detail (use of single kthread) to the userland
   visible interface in a way where we can't easily get back out of.
   For example, what if jffs needs its gc workers to be multi-threaded
   and per-NUMA for high-iops devices later on?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC PATCH 07/18] kthread: Make iterant kthreads freezable by default
From: Tejun Heo @ 2015-06-09  7:20 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
	Richard Weinberger, Steven Rostedt, David Woodhouse,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Trond Myklebust,
	Anna Schumaker, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Chris Mason,
	Paul E. McKenney, Thomas Gleixner, Linus Torvalds, Jiri Kosina,
	Borislav Petkov, Michal Hocko,
	live-patching-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1433516477-5153-8-git-send-email-pmladek-AlSwsSmVLrQ@public.gmane.org>

Hello, Petr.

On Fri, Jun 05, 2015 at 05:01:06PM +0200, Petr Mladek wrote:
> Many kthreads already calls set_freezable() before they enter the main
> cycle. One of the reasons for creating iterant kthreads is to create
> a safe point for freezing and make even more kthreads properly
> freezable. Therefore it would make sense to set all iterant
> kthreads freezable by default.

Actually, for most cases, making kthreads freezable is unnecessary and
often indicative of something going wrong.  This is a crude mechanism
which goes along the line of "if all threads are stopped, the machine
should be safe to be put into whatever state", which isn't true at all
as there usually are a lot of stuff going on asynchronously especially
when interacting with hardware.

In most cases, we want to implement proper power management callbacks
which plug new issuance of whatever work-unit the code is dealing with
and drain in-flight ones.  Whether the worker threads are frozen or
not doesn't matter once that's implemented.

It seems that people have been marking kthreads freezable w/o really
thinking about it - some of them are subtly broken due to missing
drainage of in-flight things while others simply don't need freezing
for correctness.

We do want to clean up freezer usage in the kernel but definitely do
not want to make kthreads freezable by default especially given that
the freezer essentially is one giant lockdep-less system-wide lock.

> However some kthreads might be hard to make properly freezable.
> For example, if they do non-interruptible sleeps. They would
> need to explicitly clear PF_NOFREEZE flag in the init() call.
> But it should be avoided whenever possible.

So, big no here.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC PATCH 09/18] kthread: Make it easier to correctly sleep in iterant kthreads
From: Tejun Heo @ 2015-06-09  7:32 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Peter Zijlstra, Andrew Morton, Oleg Nesterov, Ingo Molnar,
	Richard Weinberger, Steven Rostedt, David Woodhouse,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Trond Myklebust,
	Anna Schumaker, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Chris Mason,
	Paul E. McKenney, Thomas Gleixner, Linus Torvalds, Jiri Kosina,
	Borislav Petkov, Michal Hocko,
	live-patching-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150608100107.GA3135-KsEp0d+Q8qECVLCxKZUutA@public.gmane.org>

Hello,

On Mon, Jun 08, 2015 at 12:01:07PM +0200, Petr Mladek wrote:
> BTW: What is the preferred way of freezing, please? Is it better
> to end up in the fridge or is it fine to call freezer_do_not_count();
> or set PF_NOFREEZE when it is safe?

There's no one good answer.  The closest would be "don't use freezer
on kthreads".  As Peter said, exit points are always safe freezing
points and it's generally a good idea to avoid adding one anywhere
else.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC PATCH 00/18] kthreads/signal: Safer kthread API and signal handling
From: Tejun Heo @ 2015-06-09  7:58 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
	Richard Weinberger, Steven Rostedt, David Woodhouse,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Trond Myklebust,
	Anna Schumaker, linux-nfs-u79uwXL29TY76Z2rM5mHXA, Chris Mason,
	Paul E. McKenney, Thomas Gleixner, Linus Torvalds, Jiri Kosina,
	Borislav Petkov, Michal Hocko,
	live-patching-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150609061025.GU21465-qYNAdHglDFBN0TnZuCh8vA@public.gmane.org>

Hello, Petr.

I've skimmed through the patchset and I'm not quite sure.
kthread_iterant seems to map almost one to one to kthread_worker
interface.  One calls a predefined callback repeatedly while the other
queues work items which contain callback.  One does nasty plumbing
tasks inbetween interations, the other does inbetween work items.  One
has sleep helper to sleep "safely", the other can use delayed work
items and flushing cancel.  In fact, I'm pretty sure it'd be trivial
to convert between the two sets of API.

If so, is it really worthwhile to introduce the new API?
kthread_iterant is closer to raw kthread but not quite.  It shouldn't
be difficult to apply the bulk of kthread_iterant's features to
kthread_worker.  Wouldn't it be more beneficial to have async
execution mechanisms more closely aligned?

Thanks a lot.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox