[PATCH 00/10] DASD FC endpoint security

[PATCH 00/10] DASD FC endpoint security

[Stefan Haberland]

Hi Jens,

please see following patches that add a new DASD feature to show
fibre channel endpoint security.
The patches apply on you for-next branch and are intended for 5.10.

The s390/cio patches should also go upstream through your tree this time
since they are required for the DASD patches to build.

Best regards,
Stefan

Jan Höppner (7):
  s390/dasd: Remove unused parameter from dasd_generic_probe()
  s390/dasd: Move duplicate code to separate function
  s390/dasd: Store path configuration data during path handling
  s390/dasd: Fix operational path inconsistency
  s390/dasd: Display FC Endpoint Security information via sysfs
  s390/dasd: Prepare for additional path event handling
  s390/dasd: Process FCES path event notification

Sebastian Ott (1):
  s390/cio: Export information about Endpoint-Security Capability

Vineeth Vijayan (2):
  s390/cio: Provide Endpoint-Security Mode per CU
  s390/cio: Add support for FCES status notification

 arch/s390/include/asm/ccwdev.h   |   2 +
 arch/s390/include/asm/cio.h      |   1 +
 drivers/s390/block/dasd.c        |  22 ++--
 drivers/s390/block/dasd_devmap.c | 105 +++++++++++++++++++
 drivers/s390/block/dasd_eckd.c   | 170 ++++++++++++++++++++-----------
 drivers/s390/block/dasd_fba.c    |   2 +-
 drivers/s390/block/dasd_int.h    | 111 +++++++++++++++++++-
 drivers/s390/cio/chp.c           |  15 +++
 drivers/s390/cio/chp.h           |   1 +
 drivers/s390/cio/chsc.c          | 145 ++++++++++++++++++++++++--
 drivers/s390/cio/chsc.h          |   3 +-
 drivers/s390/cio/device.c        |  15 ++-
 12 files changed, 514 insertions(+), 78 deletions(-)

-- 
2.17.1

[PATCH 01/10] s390/cio: Export information about Endpoint-Security Capability

[Stefan Haberland]

From: Sebastian Ott <sebott@linux.ibm.com>

Add a new sysfs attribute 'esc' per chpid. This new attribute exports
the Endpoint-Security-Capability byte of channel-path description block,
which could be 0-None, 1-Authentication, 2 and 3-Encryption.

For example:
$ cat /sys/devices/css0/chp0.34/esc
0

Reference-ID: IO1812
Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
[vneethv@linux.ibm.com: cleaned-up & modified description]
Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Acked-by: Vasily Gorbik <gor@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/cio/chp.c  | 15 +++++++++++++++
 drivers/s390/cio/chsc.h |  3 ++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
index dfcbe54591fb..8d0de6adcad0 100644
--- a/drivers/s390/cio/chp.c
+++ b/drivers/s390/cio/chp.c
@@ -384,6 +384,20 @@ static ssize_t chp_chid_external_show(struct device *dev,
 }
 static DEVICE_ATTR(chid_external, 0444, chp_chid_external_show, NULL);
 
+static ssize_t chp_esc_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct channel_path *chp = to_channelpath(dev);
+	ssize_t rc;
+
+	mutex_lock(&chp->lock);
+	rc = sprintf(buf, "%x\n", chp->desc_fmt1.esc);
+	mutex_unlock(&chp->lock);
+
+	return rc;
+}
+static DEVICE_ATTR(esc, 0444, chp_esc_show, NULL);
+
 static ssize_t util_string_read(struct file *filp, struct kobject *kobj,
 				struct bin_attribute *attr, char *buf,
 				loff_t off, size_t count)
@@ -414,6 +428,7 @@ static struct attribute *chp_attrs[] = {
 	&dev_attr_shared.attr,
 	&dev_attr_chid.attr,
 	&dev_attr_chid_external.attr,
+	&dev_attr_esc.attr,
 	NULL,
 };
 static struct attribute_group chp_attr_group = {
diff --git a/drivers/s390/cio/chsc.h b/drivers/s390/cio/chsc.h
index 7ecf7e4c402e..4f049d17355d 100644
--- a/drivers/s390/cio/chsc.h
+++ b/drivers/s390/cio/chsc.h
@@ -27,7 +27,8 @@ struct channel_path_desc_fmt1 {
 	u8 lsn;
 	u8 desc;
 	u8 chpid;
-	u32:24;
+	u32:16;
+	u8 esc;
 	u8 chpp;
 	u32 unused[2];
 	u16 chid;
-- 
2.17.1

[PATCH 02/10] s390/cio: Provide Endpoint-Security Mode per CU

[Stefan Haberland]

From: Vineeth Vijayan <vneethv@linux.ibm.com>

Add an interface in the CIO layer to retrieve the information about the
Endpoint-Security Mode (ESM) of the specified CU. The ESM values are
defined as 0-None, 1-Authenticated or 2, 3-Encrypted.

Reference-ID: IO1812
Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
[vneethv@linux.ibm.com: cleaned-up and modified description]
Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Acked-by: Vasily Gorbik <gor@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 arch/s390/include/asm/cio.h |  1 +
 drivers/s390/cio/chsc.c     | 83 +++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/arch/s390/include/asm/cio.h b/arch/s390/include/asm/cio.h
index b5bfb3123cb1..66e06d0efb72 100644
--- a/arch/s390/include/asm/cio.h
+++ b/arch/s390/include/asm/cio.h
@@ -373,5 +373,6 @@ struct gen_pool *cio_gp_dma_create(struct device *dma_dev, int nr_pages);
 int chsc_sstpc(void *page, unsigned int op, u16 ctrl, u64 *clock_delta);
 int chsc_sstpi(void *page, void *result, size_t size);
 int chsc_sgib(u32 origin);
+int chsc_scud(u16 cu, u64 *esm, u8 *esm_valid);
 
 #endif
diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
index c314e9495c1b..513fc5748d6e 100644
--- a/drivers/s390/cio/chsc.c
+++ b/drivers/s390/cio/chsc.c
@@ -1403,3 +1403,86 @@ int chsc_sgib(u32 origin)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(chsc_sgib);
+
+#define SCUD_REQ_LEN	0x10 /* SCUD request block length */
+#define SCUD_REQ_CMD	0x4b /* SCUD Command Code */
+
+struct chse_cudb {
+	u16 flags:8;
+	u16 chp_valid:8;
+	u16 cu;
+	u32 esm_valid:8;
+	u32:24;
+	u8 chpid[8];
+	u32:32;
+	u32:32;
+	u8 esm[8];
+	u32 efla[8];
+} __packed;
+
+struct chsc_scud {
+	struct chsc_header request;
+	u16:4;
+	u16 fmt:4;
+	u16 cssid:8;
+	u16 first_cu;
+	u16:16;
+	u16 last_cu;
+	u32:32;
+	struct chsc_header response;
+	u16:4;
+	u16 fmt_resp:4;
+	u32:24;
+	struct chse_cudb cudb[];
+} __packed;
+
+/**
+ * chsc_scud() - Store control-unit description.
+ * @cu:		number of the control-unit
+ * @esm:	8 1-byte endpoint security mode values
+ * @esm_valid:	validity mask for @esm
+ *
+ * Interface to retrieve information about the endpoint security
+ * modes for up to 8 paths of a control unit.
+ *
+ * Returns 0 on success.
+ */
+int chsc_scud(u16 cu, u64 *esm, u8 *esm_valid)
+{
+	struct chsc_scud *scud = chsc_page;
+	int ret;
+
+	spin_lock_irq(&chsc_page_lock);
+	memset(chsc_page, 0, PAGE_SIZE);
+	scud->request.length = SCUD_REQ_LEN;
+	scud->request.code = SCUD_REQ_CMD;
+	scud->fmt = 0;
+	scud->cssid = 0;
+	scud->first_cu = cu;
+	scud->last_cu = cu;
+
+	ret = chsc(scud);
+	if (!ret)
+		ret = chsc_error_from_response(scud->response.code);
+
+	if (!ret && (scud->response.length <= 8 || scud->fmt_resp != 0
+			|| !(scud->cudb[0].flags & 0x80)
+			|| scud->cudb[0].cu != cu)) {
+
+		CIO_MSG_EVENT(2, "chsc: scud failed rc=%04x, L2=%04x "
+			"FMT=%04x, cudb.flags=%02x, cudb.cu=%04x",
+			scud->response.code, scud->response.length,
+			scud->fmt_resp, scud->cudb[0].flags, scud->cudb[0].cu);
+		ret = -EINVAL;
+	}
+
+	if (ret)
+		goto out;
+
+	memcpy(esm, scud->cudb[0].esm, sizeof(*esm));
+	*esm_valid = scud->cudb[0].esm_valid;
+out:
+	spin_unlock_irq(&chsc_page_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(chsc_scud);
-- 
2.17.1

[PATCH 03/10] s390/cio: Add support for FCES status notification

[Stefan Haberland]

From: Vineeth Vijayan <vneethv@linux.ibm.com>

Fibre Channel Endpoint-Security event is received as an sei:nt0 type
in the CIO layer. This information needs to be shared with the
CCW device drivers using the path_events callback.

Reference-ID: IO1812
Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
Co-developed-by: Sebastian Ott <sebott@linux.ibm.com>
Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Acked-by: Vasily Gorbik <gor@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 arch/s390/include/asm/ccwdev.h |  2 ++
 drivers/s390/cio/chp.h         |  1 +
 drivers/s390/cio/chsc.c        | 62 +++++++++++++++++++++++++++++-----
 drivers/s390/cio/device.c      | 15 +++++++-
 4 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/arch/s390/include/asm/ccwdev.h b/arch/s390/include/asm/ccwdev.h
index 3cfe1eb89838..14e1bbfd969e 100644
--- a/arch/s390/include/asm/ccwdev.h
+++ b/arch/s390/include/asm/ccwdev.h
@@ -104,6 +104,8 @@ struct ccw_device {
 					       was successfully verified. */
 #define PE_PATHGROUP_ESTABLISHED	0x4 /* A pathgroup was reset and had
 					       to be established again. */
+#define PE_PATH_FCES_EVENT		0x8 /* The FCES Status of a path has
+					     * changed. */
 
 /*
  * Possible CIO actions triggered by the unit check handler.
diff --git a/drivers/s390/cio/chp.h b/drivers/s390/cio/chp.h
index 20259f3fbf45..7ee9eba0abcb 100644
--- a/drivers/s390/cio/chp.h
+++ b/drivers/s390/cio/chp.h
@@ -23,6 +23,7 @@
 #define CHP_OFFLINE 1
 #define CHP_VARY_ON 2
 #define CHP_VARY_OFF 3
+#define CHP_FCES_EVENT 4
 
 struct chp_link {
 	struct chp_id chpid;
diff --git a/drivers/s390/cio/chsc.c b/drivers/s390/cio/chsc.c
index 513fc5748d6e..04fbcfcf9e42 100644
--- a/drivers/s390/cio/chsc.c
+++ b/drivers/s390/cio/chsc.c
@@ -37,6 +37,9 @@ static void *sei_page;
 static void *chsc_page;
 static DEFINE_SPINLOCK(chsc_page_lock);
 
+#define SEI_VF_FLA	0xc0 /* VF flag for Full Link Address */
+#define SEI_RS_CHPID	0x4  /* 4 in RS field indicates CHPID */
+
 /**
  * chsc_error_from_response() - convert a chsc response to an error
  * @response: chsc response code
@@ -285,6 +288,15 @@ static void s390_process_res_acc(struct chp_link *link)
 	css_schedule_reprobe();
 }
 
+static int process_fces_event(struct subchannel *sch, void *data)
+{
+	spin_lock_irq(sch->lock);
+	if (sch->driver && sch->driver->chp_event)
+		sch->driver->chp_event(sch, data, CHP_FCES_EVENT);
+	spin_unlock_irq(sch->lock);
+	return 0;
+}
+
 struct chsc_sei_nt0_area {
 	u8  flags;
 	u8  vf;				/* validity flags */
@@ -362,6 +374,16 @@ static char *store_ebcdic(char *dest, const char *src, unsigned long len,
 	return dest + len;
 }
 
+static void chsc_link_from_sei(struct chp_link *link,
+				struct chsc_sei_nt0_area *sei_area)
+{
+	if ((sei_area->vf & SEI_VF_FLA) != 0) {
+		link->fla	= sei_area->fla;
+		link->fla_mask	= ((sei_area->vf & SEI_VF_FLA) == SEI_VF_FLA) ?
+							0xffff : 0xff00;
+	}
+}
+
 /* Format node ID and parameters for output in LIR log message. */
 static void format_node_data(char *params, char *id, struct node_descriptor *nd)
 {
@@ -451,15 +473,7 @@ static void chsc_process_sei_res_acc(struct chsc_sei_nt0_area *sei_area)
 	}
 	memset(&link, 0, sizeof(struct chp_link));
 	link.chpid = chpid;
-	if ((sei_area->vf & 0xc0) != 0) {
-		link.fla = sei_area->fla;
-		if ((sei_area->vf & 0xc0) == 0xc0)
-			/* full link address */
-			link.fla_mask = 0xffff;
-		else
-			/* link address */
-			link.fla_mask = 0xff00;
-	}
+	chsc_link_from_sei(&link, sei_area);
 	s390_process_res_acc(&link);
 }
 
@@ -568,6 +582,33 @@ static void chsc_process_sei_ap_cfg_chg(struct chsc_sei_nt0_area *sei_area)
 	ap_bus_cfg_chg();
 }
 
+static void chsc_process_sei_fces_event(struct chsc_sei_nt0_area *sei_area)
+{
+	struct chp_link link;
+	struct chp_id chpid;
+	struct channel_path *chp;
+
+	CIO_CRW_EVENT(4,
+	"chsc: FCES status notification (rs=%02x, rs_id=%04x, FCES-status=%x)\n",
+		sei_area->rs, sei_area->rsid, sei_area->ccdf[0]);
+
+	if (sei_area->rs != SEI_RS_CHPID)
+		return;
+	chp_id_init(&chpid);
+	chpid.id = sei_area->rsid;
+
+	/* Ignore the event on unknown/invalid chp */
+	chp = chpid_to_chp(chpid);
+	if (!chp)
+		return;
+
+	memset(&link, 0, sizeof(struct chp_link));
+	link.chpid = chpid;
+	chsc_link_from_sei(&link, sei_area);
+
+	for_each_subchannel_staged(process_fces_event, NULL, &link);
+}
+
 static void chsc_process_sei_nt2(struct chsc_sei_nt2_area *sei_area)
 {
 	switch (sei_area->cc) {
@@ -609,6 +650,9 @@ static void chsc_process_sei_nt0(struct chsc_sei_nt0_area *sei_area)
 	case 14: /* scm available notification */
 		chsc_process_sei_scm_avail(sei_area);
 		break;
+	case 15: /* FCES event notification */
+		chsc_process_sei_fces_event(sei_area);
+		break;
 	default: /* other stuff */
 		CIO_CRW_EVENT(2, "chsc: sei nt0 unhandled cc=%d\n",
 			      sei_area->cc);
diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
index b29fe8d50baf..aab13c78db9f 100644
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -1170,7 +1170,8 @@ static int io_subchannel_chp_event(struct subchannel *sch,
 				   struct chp_link *link, int event)
 {
 	struct ccw_device *cdev = sch_get_cdev(sch);
-	int mask;
+	int mask, chpid, valid_bit;
+	int path_event[8];
 
 	mask = chp_ssd_get_mask(&sch->ssd_info, link);
 	if (!mask)
@@ -1205,6 +1206,18 @@ static int io_subchannel_chp_event(struct subchannel *sch,
 			cdev->private->path_new_mask |= mask;
 		io_subchannel_verify(sch);
 		break;
+	case CHP_FCES_EVENT:
+		/* Forward Endpoint Security event */
+		for (chpid = 0, valid_bit = 0x80; chpid < 8; chpid++,
+				valid_bit >>= 1) {
+			if (mask & valid_bit)
+				path_event[chpid] = PE_PATH_FCES_EVENT;
+			else
+				path_event[chpid] = PE_NONE;
+		}
+		if (cdev)
+			cdev->drv->path_event(cdev, path_event);
+		break;
 	}
 	return 0;
 }
-- 
2.17.1

[PATCH 04/10] s390/dasd: Remove unused parameter from dasd_generic_probe()

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

The discipline argument in dasd_generic_probe() isn't used and there is
no history how it was used in the past. Remove it.

Reference-ID: IO1812
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c      | 3 +--
 drivers/s390/block/dasd_eckd.c | 2 +-
 drivers/s390/block/dasd_fba.c  | 2 +-
 drivers/s390/block/dasd_int.h  | 2 +-
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index eb17fea8075c..9a0300d2e744 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -3465,8 +3465,7 @@ static void dasd_generic_auto_online(void *data, async_cookie_t cookie)
  * Initial attempt at a probe function. this can be simplified once
  * the other detection code is gone.
  */
-int dasd_generic_probe(struct ccw_device *cdev,
-		       struct dasd_discipline *discipline)
+int dasd_generic_probe(struct ccw_device *cdev)
 {
 	int ret;
 
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index ad44d22e8859..2b39d2a5965f 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -143,7 +143,7 @@ dasd_eckd_probe (struct ccw_device *cdev)
 				"ccw-device options");
 		return ret;
 	}
-	ret = dasd_generic_probe(cdev, &dasd_eckd_discipline);
+	ret = dasd_generic_probe(cdev);
 	return ret;
 }
 
diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c
index 1a44e321b54e..5b0ebf6bf20f 100644
--- a/drivers/s390/block/dasd_fba.c
+++ b/drivers/s390/block/dasd_fba.c
@@ -58,7 +58,7 @@ static struct ccw_driver dasd_fba_driver; /* see below */
 static int
 dasd_fba_probe(struct ccw_device *cdev)
 {
-	return dasd_generic_probe(cdev, &dasd_fba_discipline);
+	return dasd_generic_probe(cdev);
 }
 
 static int
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index fa552f9f1666..6dce9d4c55ce 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -774,7 +774,7 @@ void dasd_block_set_timer(struct dasd_block *, int);
 void dasd_block_clear_timer(struct dasd_block *);
 int  dasd_cancel_req(struct dasd_ccw_req *);
 int dasd_flush_device_queue(struct dasd_device *);
-int dasd_generic_probe (struct ccw_device *, struct dasd_discipline *);
+int dasd_generic_probe(struct ccw_device *);
 void dasd_generic_free_discipline(struct dasd_device *);
 void dasd_generic_remove (struct ccw_device *cdev);
 int dasd_generic_set_online(struct ccw_device *, struct dasd_discipline *);
-- 
2.17.1

[PATCH 05/10] s390/dasd: Move duplicate code to separate function

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

For storing retrieved path information both the if and else block in
dasd_eckd_read_conf() use the same code. To avoid duplicate code this
should be done after the if/else block. To further increase readability,
move the code to a new function, dasd_eckd_store_conf_data().

Reference-ID: IO1812
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 42 ++++++++++++++++------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 2b39d2a5965f..497aa81778b6 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1000,6 +1000,22 @@ static unsigned char dasd_eckd_path_access(void *conf_data, int conf_len)
 		return 0;
 }
 
+static void dasd_eckd_store_conf_data(struct dasd_device *device,
+				      struct dasd_conf_data *conf_data, int chp)
+{
+	struct channel_path_desc_fmt0 *chp_desc;
+	struct subchannel_id sch_id;
+
+	ccw_device_get_schid(device->cdev, &sch_id);
+	device->path[chp].conf_data = conf_data;
+	device->path[chp].cssid = sch_id.cssid;
+	device->path[chp].ssid = sch_id.ssid;
+	chp_desc = ccw_device_get_chp_desc(device->cdev, chp);
+	if (chp_desc)
+		device->path[chp].chpid = chp_desc->chpid;
+	kfree(chp_desc);
+}
+
 static void dasd_eckd_clear_conf_data(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
@@ -1016,7 +1032,6 @@ static void dasd_eckd_clear_conf_data(struct dasd_device *device)
 	}
 }
 
-
 static int dasd_eckd_read_conf(struct dasd_device *device)
 {
 	void *conf_data;
@@ -1026,12 +1041,9 @@ static int dasd_eckd_read_conf(struct dasd_device *device)
 	struct dasd_eckd_private *private, path_private;
 	struct dasd_uid *uid;
 	char print_path_uid[60], print_device_uid[60];
-	struct channel_path_desc_fmt0 *chp_desc;
-	struct subchannel_id sch_id;
 
 	private = device->private;
 	opm = ccw_device_get_path_mask(device->cdev);
-	ccw_device_get_schid(device->cdev, &sch_id);
 	conf_data_saved = 0;
 	path_err = 0;
 	/* get configuration data per operational path */
@@ -1066,15 +1078,6 @@ static int dasd_eckd_read_conf(struct dasd_device *device)
 				kfree(conf_data);
 				continue;
 			}
-			pos = pathmask_to_pos(lpm);
-			/* store per path conf_data */
-			device->path[pos].conf_data = conf_data;
-			device->path[pos].cssid = sch_id.cssid;
-			device->path[pos].ssid = sch_id.ssid;
-			chp_desc = ccw_device_get_chp_desc(device->cdev, pos);
-			if (chp_desc)
-				device->path[pos].chpid = chp_desc->chpid;
-			kfree(chp_desc);
 			/*
 			 * build device UID that other path data
 			 * can be compared to it
@@ -1132,18 +1135,13 @@ static int dasd_eckd_read_conf(struct dasd_device *device)
 				dasd_path_add_cablepm(device, lpm);
 				continue;
 			}
-			pos = pathmask_to_pos(lpm);
-			/* store per path conf_data */
-			device->path[pos].conf_data = conf_data;
-			device->path[pos].cssid = sch_id.cssid;
-			device->path[pos].ssid = sch_id.ssid;
-			chp_desc = ccw_device_get_chp_desc(device->cdev, pos);
-			if (chp_desc)
-				device->path[pos].chpid = chp_desc->chpid;
-			kfree(chp_desc);
 			path_private.conf_data = NULL;
 			path_private.conf_len = 0;
 		}
+
+		pos = pathmask_to_pos(lpm);
+		dasd_eckd_store_conf_data(device, conf_data, pos);
+
 		switch (dasd_eckd_path_access(conf_data, conf_len)) {
 		case 0x02:
 			dasd_path_add_nppm(device, lpm);
-- 
2.17.1

[PATCH 06/10] s390/dasd: Store path configuration data during path handling

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

Currently, the configuration data for a path is retrieved during a path
verification and used only temporarily. If a path is newly added to the
I/O setup after a boot, no configuration data will be stored for this
particular path.
However, this data is required for later use and should be present for
a valid I/O path anyway. Store this data during the path verification so
that newly added paths can provide all information necessary.

Reference-ID: IO1812
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 497aa81778b6..274f79869b7c 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1263,9 +1263,10 @@ static void do_path_verification_work(struct work_struct *work)
 	struct dasd_uid *uid;
 	__u8 path_rcd_buf[DASD_ECKD_RCD_DATA_SIZE];
 	__u8 lpm, opm, npm, ppm, epm, hpfpm, cablepm;
+	struct dasd_conf_data *conf_data;
 	unsigned long flags;
 	char print_uid[60];
-	int rc;
+	int rc, pos;
 
 	data = container_of(work, struct path_verification_work_data, worker);
 	device = data->device;
@@ -1395,6 +1396,14 @@ static void do_path_verification_work(struct work_struct *work)
 			}
 		}
 
+		conf_data = kzalloc(DASD_ECKD_RCD_DATA_SIZE, GFP_KERNEL);
+		if (conf_data) {
+			memcpy(conf_data, data->rcd_buffer,
+			       DASD_ECKD_RCD_DATA_SIZE);
+		}
+		pos = pathmask_to_pos(lpm);
+		dasd_eckd_store_conf_data(device, conf_data, pos);
+
 		/*
 		 * There is a small chance that a path is lost again between
 		 * above path verification and the following modification of
-- 
2.17.1

[PATCH 07/10] s390/dasd: Fix operational path inconsistency

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

During online processing and setting up a DASD device, the configuration
data for operational paths is read and validated two times
(dasd_eckd_read_conf()). The first time to provide information that are
necessary for the LCU setup. A second time after the LCU setup as a
device might report different configuration data then.

When the configuration setup for each operational path is being
validated, an initial call to dasd_eckd_clear_conf_data() is issued.
This call wipes all previously available configuration data and path
information for each path.
However, the operational path mask is not updated during this process.

As a result, the stored operational path mask might no longer correspond
to the operational paths mask reported by the CIO layer, as several
paths might be gone between the two dasd_eckd_read_conf() calls.

This inconsistency leads to more severe issues in later path handling
changes. Fix this by removing the channel paths from the operational
path mask during the dasd_eckd_clear_conf_data() call.

Reference-ID: IO1812
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 274f79869b7c..271768e4606c 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1029,6 +1029,7 @@ static void dasd_eckd_clear_conf_data(struct dasd_device *device)
 		device->path[i].cssid = 0;
 		device->path[i].ssid = 0;
 		device->path[i].chpid = 0;
+		dasd_path_notoper(device, i);
 	}
 }
 
-- 
2.17.1

[PATCH 08/10] s390/dasd: Display FC Endpoint Security information via sysfs

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

Add a new sysfs attribute (fc_security) per device and per operational
channel path. The information of the current FC Endpoint Security state
is received through the CIO layer.

The state of the FC Endpoint Security can be either "Unsupported",
"Authentication", or "Encryption".

For example:
$ cat /sys/bus/ccw/devices/0.0.c600/fc_security
Encryption

If any of the operational paths is in a state different from all
others, the device sysfs attribute will display the additional state
"Inconsistent".

The sysfs attributes per paths are organised in a new directory called
"paths_info" with subdirectories for each path.

/sys/bus/ccw/devices/0.0.c600/paths_info/
├── 0.38
│   └── fc_security
├── 0.39
│   └── fc_security
├── 0.3a
│   └── fc_security
└── 0.3b
    └── fc_security

Reference-ID: IO1812
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_devmap.c | 105 +++++++++++++++++++++++++++++++
 drivers/s390/block/dasd_eckd.c   |  30 +++++++++
 drivers/s390/block/dasd_int.h    |  68 ++++++++++++++++++++
 3 files changed, 203 insertions(+)

diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index 32fc51341d99..d4af087ecfe7 100644
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -576,6 +576,11 @@ dasd_create_device(struct ccw_device *cdev)
 	dev_set_drvdata(&cdev->dev, device);
 	spin_unlock_irqrestore(get_ccwdev_lock(cdev), flags);
 
+	device->paths_info = kset_create_and_add("paths_info", NULL,
+						 &device->cdev->dev.kobj);
+	if (!device->paths_info)
+		dev_warn(&cdev->dev, "Could not create paths_info kset\n");
+
 	return device;
 }
 
@@ -622,6 +627,9 @@ dasd_delete_device(struct dasd_device *device)
 	wait_event(dasd_delete_wq, atomic_read(&device->ref_count) == 0);
 
 	dasd_generic_free_discipline(device);
+
+	kset_unregister(device->paths_info);
+
 	/* Disconnect dasd_device structure from ccw_device structure. */
 	cdev = device->cdev;
 	device->cdev = NULL;
@@ -1641,6 +1649,39 @@ dasd_path_interval_store(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(path_interval, 0644, dasd_path_interval_show,
 		   dasd_path_interval_store);
 
+static ssize_t
+dasd_device_fcs_show(struct device *dev, struct device_attribute *attr,
+		     char *buf)
+{
+	struct dasd_device *device;
+	int fc_sec;
+	int rc;
+
+	device = dasd_device_from_cdev(to_ccwdev(dev));
+	if (IS_ERR(device))
+		return -ENODEV;
+	fc_sec = dasd_path_get_fcs_device(device);
+	if (fc_sec == -EINVAL)
+		rc = snprintf(buf, PAGE_SIZE, "Inconsistent\n");
+	else
+		rc = snprintf(buf, PAGE_SIZE, "%s\n", dasd_path_get_fcs_str(fc_sec));
+	dasd_put_device(device);
+
+	return rc;
+}
+static DEVICE_ATTR(fc_security, 0444, dasd_device_fcs_show, NULL);
+
+static ssize_t
+dasd_path_fcs_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	struct dasd_path *path = to_dasd_path(kobj);
+	unsigned int fc_sec = path->fc_security;
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", dasd_path_get_fcs_str(fc_sec));
+}
+
+static struct kobj_attribute path_fcs_attribute =
+	__ATTR(fc_security, 0444, dasd_path_fcs_show, NULL);
 
 #define DASD_DEFINE_ATTR(_name, _func)					\
 static ssize_t dasd_##_name##_show(struct device *dev,			\
@@ -1697,6 +1738,7 @@ static struct attribute * dasd_attrs[] = {
 	&dev_attr_path_reset.attr,
 	&dev_attr_hpf.attr,
 	&dev_attr_ese.attr,
+	&dev_attr_fc_security.attr,
 	NULL,
 };
 
@@ -1777,6 +1819,69 @@ dasd_set_feature(struct ccw_device *cdev, int feature, int flag)
 }
 EXPORT_SYMBOL(dasd_set_feature);
 
+static struct attribute *paths_info_attrs[] = {
+	&path_fcs_attribute.attr,
+	NULL,
+};
+
+static struct kobj_type path_attr_type = {
+	.release	= dasd_path_release,
+	.default_attrs	= paths_info_attrs,
+	.sysfs_ops	= &kobj_sysfs_ops,
+};
+
+static void dasd_path_init_kobj(struct dasd_device *device, int chp)
+{
+	device->path[chp].kobj.kset = device->paths_info;
+	kobject_init(&device->path[chp].kobj, &path_attr_type);
+}
+
+void dasd_path_create_kobj(struct dasd_device *device, int chp)
+{
+	int rc;
+
+	if (test_bit(DASD_FLAG_OFFLINE, &device->flags))
+		return;
+	if (!device->paths_info) {
+		dev_warn(&device->cdev->dev, "Unable to create paths objects\n");
+		return;
+	}
+	if (device->path[chp].in_sysfs)
+		return;
+	if (!device->path[chp].conf_data)
+		return;
+
+	dasd_path_init_kobj(device, chp);
+
+	rc = kobject_add(&device->path[chp].kobj, NULL, "%x.%02x",
+			 device->path[chp].cssid, device->path[chp].chpid);
+	if (rc)
+		kobject_put(&device->path[chp].kobj);
+	device->path[chp].in_sysfs = true;
+}
+EXPORT_SYMBOL(dasd_path_create_kobj);
+
+void dasd_path_create_kobjects(struct dasd_device *device)
+{
+	u8 lpm, opm;
+
+	opm = dasd_path_get_opm(device);
+	for (lpm = 0x80; lpm; lpm >>= 1) {
+		if (!(lpm & opm))
+			continue;
+		dasd_path_create_kobj(device, pathmask_to_pos(lpm));
+	}
+}
+EXPORT_SYMBOL(dasd_path_create_kobjects);
+
+void dasd_path_remove_kobj(struct dasd_device *device, int chp)
+{
+	if (device->path[chp].in_sysfs) {
+		kobject_put(&device->path[chp].kobj);
+		device->path[chp].in_sysfs = false;
+	}
+}
+EXPORT_SYMBOL(dasd_path_remove_kobj);
 
 int dasd_add_sysfs_files(struct ccw_device *cdev)
 {
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 271768e4606c..63061ff5ecab 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1030,6 +1030,30 @@ static void dasd_eckd_clear_conf_data(struct dasd_device *device)
 		device->path[i].ssid = 0;
 		device->path[i].chpid = 0;
 		dasd_path_notoper(device, i);
+		dasd_path_remove_kobj(device, i);
+	}
+}
+
+static void dasd_eckd_read_fc_security(struct dasd_device *device)
+{
+	struct dasd_eckd_private *private = device->private;
+	u8 esm_valid;
+	u8 esm[8];
+	int chp;
+	int rc;
+
+	rc = chsc_scud(private->uid.ssid, (u64 *)esm, &esm_valid);
+	if (rc) {
+		for (chp = 0; chp < 8; chp++)
+			device->path[chp].fc_security = 0;
+		return;
+	}
+
+	for (chp = 0; chp < 8; chp++) {
+		if (esm_valid & (0x80 >> chp))
+			device->path[chp].fc_security = esm[chp];
+		else
+			device->path[chp].fc_security = 0;
 	}
 }
 
@@ -1159,6 +1183,8 @@ static int dasd_eckd_read_conf(struct dasd_device *device)
 		}
 	}
 
+	dasd_eckd_read_fc_security(device);
+
 	return path_err;
 }
 
@@ -1425,6 +1451,8 @@ static void do_path_verification_work(struct work_struct *work)
 		dasd_path_add_cablepm(device, cablepm);
 		dasd_path_add_nohpfpm(device, hpfpm);
 		spin_unlock_irqrestore(get_ccwdev_lock(device->cdev), flags);
+
+		dasd_path_create_kobj(device, pos);
 	}
 	clear_bit(DASD_FLAG_PATH_VERIFY, &device->flags);
 	dasd_put_device(device);
@@ -2064,6 +2092,8 @@ dasd_eckd_check_characteristics(struct dasd_device *device)
 	if (rc)
 		goto out_err3;
 
+	dasd_path_create_kobjects(device);
+
 	/* Read Feature Codes */
 	dasd_eckd_read_features(device);
 
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index 6dce9d4c55ce..93d9cc938924 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -426,6 +426,35 @@ extern struct dasd_discipline *dasd_diag_discipline_pointer;
 #define DASD_THRHLD_MAX		4294967295U
 #define DASD_INTERVAL_MAX	4294967295U
 
+/* FC Endpoint Security Capabilities */
+#define DASD_FC_SECURITY_UNSUP		0
+#define DASD_FC_SECURITY_AUTH		1
+#define DASD_FC_SECURITY_ENC_FCSP2	2
+#define DASD_FC_SECURITY_ENC_ERAS	3
+
+#define DASD_FC_SECURITY_ENC_STR	"Encryption"
+static const struct {
+	u8 value;
+	char *name;
+} dasd_path_fcs_mnemonics[] = {
+	{ DASD_FC_SECURITY_UNSUP,	"Unsupported" },
+	{ DASD_FC_SECURITY_AUTH,	"Authentication" },
+	{ DASD_FC_SECURITY_ENC_FCSP2,	DASD_FC_SECURITY_ENC_STR },
+	{ DASD_FC_SECURITY_ENC_ERAS,	DASD_FC_SECURITY_ENC_STR },
+};
+
+static inline char *dasd_path_get_fcs_str(int val)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dasd_path_fcs_mnemonics); i++) {
+		if (dasd_path_fcs_mnemonics[i].value == val)
+			return dasd_path_fcs_mnemonics[i].name;
+	}
+
+	return dasd_path_fcs_mnemonics[0].name;
+}
+
 struct dasd_path {
 	unsigned long flags;
 	u8 cssid;
@@ -434,8 +463,18 @@ struct dasd_path {
 	struct dasd_conf_data *conf_data;
 	atomic_t error_count;
 	unsigned long errorclk;
+	u8 fc_security;
+	struct kobject kobj;
+	bool in_sysfs;
 };
 
+#define to_dasd_path(path) container_of(path, struct dasd_path, kobj)
+
+static inline void dasd_path_release(struct kobject *kobj)
+{
+/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
+}
+
 
 struct dasd_profile_info {
 	/* legacy part of profile data, as in dasd_profile_info_t */
@@ -547,6 +586,7 @@ struct dasd_device {
 	struct dentry *hosts_dentry;
 	struct dasd_profile profile;
 	struct dasd_format_entry format_entry;
+	struct kset *paths_info;
 };
 
 struct dasd_block {
@@ -824,6 +864,9 @@ int dasd_set_feature(struct ccw_device *, int, int);
 
 int dasd_add_sysfs_files(struct ccw_device *);
 void dasd_remove_sysfs_files(struct ccw_device *);
+void dasd_path_create_kobj(struct dasd_device *, int);
+void dasd_path_create_kobjects(struct dasd_device *);
+void dasd_path_remove_kobj(struct dasd_device *, int);
 
 struct dasd_device *dasd_device_from_cdev(struct ccw_device *);
 struct dasd_device *dasd_device_from_cdev_locked(struct ccw_device *);
@@ -1113,6 +1156,31 @@ static inline __u8 dasd_path_get_hpfpm(struct dasd_device *device)
 	return hpfpm;
 }
 
+static inline u8 dasd_path_get_fcs_path(struct dasd_device *device, int chp)
+{
+	return device->path[chp].fc_security;
+}
+
+static inline int dasd_path_get_fcs_device(struct dasd_device *device)
+{
+	u8 fc_sec = 0;
+	int chp;
+
+	for (chp = 0; chp < 8; chp++) {
+		if (device->opm & (0x80 >> chp)) {
+			fc_sec = device->path[chp].fc_security;
+			break;
+		}
+	}
+	for (; chp < 8; chp++) {
+		if (device->opm & (0x80 >> chp))
+			if (device->path[chp].fc_security != fc_sec)
+				return -EINVAL;
+	}
+
+	return fc_sec;
+}
+
 /*
  * add functions for path masks
  * the existing path mask will be extended by the given path mask
-- 
2.17.1

[PATCH 09/10] s390/dasd: Prepare for additional path event handling

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

As more path events need to be handled for ECKD the current path
verification infrastructure can be reused. Rename all path verifcation
code to fit the more broadly based task of path event handling and put
the path verification in a new separate function.

Reference-ID: IO1812
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c      |  4 +-
 drivers/s390/block/dasd_eckd.c | 78 +++++++++++++++++++---------------
 drivers/s390/block/dasd_int.h  |  1 +
 3 files changed, 47 insertions(+), 36 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 9a0300d2e744..97c11d557052 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -2115,8 +2115,8 @@ static void __dasd_device_check_path_events(struct dasd_device *device)
 	if (device->stopped &
 	    ~(DASD_STOPPED_DC_WAIT | DASD_UNRESUMED_PM))
 		return;
-	rc = device->discipline->verify_path(device,
-					     dasd_path_get_tbvpm(device));
+	rc = device->discipline->pe_handler(device,
+					    dasd_path_get_tbvpm(device));
 	if (rc)
 		dasd_device_set_timer(device, 50);
 	else
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 63061ff5ecab..822a8ad4e338 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -103,7 +103,7 @@ struct ext_pool_exhaust_work_data {
 };
 
 /* definitions for the path verification worker */
-struct path_verification_work_data {
+struct pe_handler_work_data {
 	struct work_struct worker;
 	struct dasd_device *device;
 	struct dasd_ccw_req cqr;
@@ -112,8 +112,8 @@ struct path_verification_work_data {
 	int isglobal;
 	__u8 tbvpm;
 };
-static struct path_verification_work_data *path_verification_worker;
-static DEFINE_MUTEX(dasd_path_verification_mutex);
+static struct pe_handler_work_data *pe_handler_worker;
+static DEFINE_MUTEX(dasd_pe_handler_mutex);
 
 struct check_attention_work_data {
 	struct work_struct worker;
@@ -1244,7 +1244,7 @@ static int verify_fcx_max_data(struct dasd_device *device, __u8 lpm)
 }
 
 static int rebuild_device_uid(struct dasd_device *device,
-			      struct path_verification_work_data *data)
+			      struct pe_handler_work_data *data)
 {
 	struct dasd_eckd_private *private = device->private;
 	__u8 lpm, opm = dasd_path_get_opm(device);
@@ -1282,10 +1282,9 @@ static int rebuild_device_uid(struct dasd_device *device,
 	return rc;
 }
 
-static void do_path_verification_work(struct work_struct *work)
+static void dasd_eckd_path_available_action(struct dasd_device *device,
+					    struct pe_handler_work_data *data)
 {
-	struct path_verification_work_data *data;
-	struct dasd_device *device;
 	struct dasd_eckd_private path_private;
 	struct dasd_uid *uid;
 	__u8 path_rcd_buf[DASD_ECKD_RCD_DATA_SIZE];
@@ -1295,19 +1294,6 @@ static void do_path_verification_work(struct work_struct *work)
 	char print_uid[60];
 	int rc, pos;
 
-	data = container_of(work, struct path_verification_work_data, worker);
-	device = data->device;
-
-	/* delay path verification until device was resumed */
-	if (test_bit(DASD_FLAG_SUSPENDED, &device->flags)) {
-		schedule_work(work);
-		return;
-	}
-	/* check if path verification already running and delay if so */
-	if (test_and_set_bit(DASD_FLAG_PATH_VERIFY, &device->flags)) {
-		schedule_work(work);
-		return;
-	}
 	opm = 0;
 	npm = 0;
 	ppm = 0;
@@ -1454,30 +1440,54 @@ static void do_path_verification_work(struct work_struct *work)
 
 		dasd_path_create_kobj(device, pos);
 	}
+}
+
+static void do_pe_handler_work(struct work_struct *work)
+{
+	struct pe_handler_work_data *data;
+	struct dasd_device *device;
+
+	data = container_of(work, struct pe_handler_work_data, worker);
+	device = data->device;
+
+	/* delay path verification until device was resumed */
+	if (test_bit(DASD_FLAG_SUSPENDED, &device->flags)) {
+		schedule_work(work);
+		return;
+	}
+	/* check if path verification already running and delay if so */
+	if (test_and_set_bit(DASD_FLAG_PATH_VERIFY, &device->flags)) {
+		schedule_work(work);
+		return;
+	}
+
+	dasd_eckd_path_available_action(device, data);
+
 	clear_bit(DASD_FLAG_PATH_VERIFY, &device->flags);
 	dasd_put_device(device);
 	if (data->isglobal)
-		mutex_unlock(&dasd_path_verification_mutex);
+		mutex_unlock(&dasd_pe_handler_mutex);
 	else
 		kfree(data);
 }
 
-static int dasd_eckd_verify_path(struct dasd_device *device, __u8 lpm)
+static int dasd_eckd_pe_handler(struct dasd_device *device, __u8 lpm)
 {
-	struct path_verification_work_data *data;
+	struct pe_handler_work_data *data;
 
 	data = kmalloc(sizeof(*data), GFP_ATOMIC | GFP_DMA);
 	if (!data) {
-		if (mutex_trylock(&dasd_path_verification_mutex)) {
-			data = path_verification_worker;
+		if (mutex_trylock(&dasd_pe_handler_mutex)) {
+			data = pe_handler_worker;
 			data->isglobal = 1;
-		} else
+		} else {
 			return -ENOMEM;
+		}
 	} else {
 		memset(data, 0, sizeof(*data));
 		data->isglobal = 0;
 	}
-	INIT_WORK(&data->worker, do_path_verification_work);
+	INIT_WORK(&data->worker, do_pe_handler_work);
 	dasd_get_device(device);
 	data->device = device;
 	data->tbvpm = lpm;
@@ -6720,7 +6730,7 @@ static struct dasd_discipline dasd_eckd_discipline = {
 	.check_device = dasd_eckd_check_characteristics,
 	.uncheck_device = dasd_eckd_uncheck_device,
 	.do_analysis = dasd_eckd_do_analysis,
-	.verify_path = dasd_eckd_verify_path,
+	.pe_handler = dasd_eckd_pe_handler,
 	.basic_to_ready = dasd_eckd_basic_to_ready,
 	.online_to_ready = dasd_eckd_online_to_ready,
 	.basic_to_known = dasd_eckd_basic_to_known,
@@ -6781,16 +6791,16 @@ dasd_eckd_init(void)
 				    GFP_KERNEL | GFP_DMA);
 	if (!dasd_vol_info_req)
 		return -ENOMEM;
-	path_verification_worker = kmalloc(sizeof(*path_verification_worker),
-				   GFP_KERNEL | GFP_DMA);
-	if (!path_verification_worker) {
+	pe_handler_worker = kmalloc(sizeof(*pe_handler_worker),
+				    GFP_KERNEL | GFP_DMA);
+	if (!pe_handler_worker) {
 		kfree(dasd_reserve_req);
 		kfree(dasd_vol_info_req);
 		return -ENOMEM;
 	}
 	rawpadpage = (void *)__get_free_page(GFP_KERNEL);
 	if (!rawpadpage) {
-		kfree(path_verification_worker);
+		kfree(pe_handler_worker);
 		kfree(dasd_reserve_req);
 		kfree(dasd_vol_info_req);
 		return -ENOMEM;
@@ -6799,7 +6809,7 @@ dasd_eckd_init(void)
 	if (!ret)
 		wait_for_device_probe();
 	else {
-		kfree(path_verification_worker);
+		kfree(pe_handler_worker);
 		kfree(dasd_reserve_req);
 		kfree(dasd_vol_info_req);
 		free_page((unsigned long)rawpadpage);
@@ -6811,7 +6821,7 @@ static void __exit
 dasd_eckd_cleanup(void)
 {
 	ccw_driver_unregister(&dasd_eckd_driver);
-	kfree(path_verification_worker);
+	kfree(pe_handler_worker);
 	kfree(dasd_reserve_req);
 	free_page((unsigned long)rawpadpage);
 }
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index 93d9cc938924..3ae1c8d42b8a 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -298,6 +298,7 @@ struct dasd_discipline {
 	 * configuration.
 	 */
 	int (*verify_path)(struct dasd_device *, __u8);
+	int (*pe_handler)(struct dasd_device *, __u8);
 
 	/*
 	 * Last things to do when a device is set online, and first things
-- 
2.17.1

[PATCH 10/10] s390/dasd: Process FCES path event notification

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

If the Fibre Channel Endpoint-Security status of a path changes, a
corresponding path event is received from the CIO layer.

Process this event by re-reading the FCES information.

As the information is retrieved for all paths on a single CU in one
call, the internal status can also be updated for all paths and no
processing per path is necessary.

Reference-ID: IO1812
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c      | 19 +++++++++++----
 drivers/s390/block/dasd_eckd.c | 12 +++++++---
 drivers/s390/block/dasd_int.h  | 42 +++++++++++++++++++++++++++++++++-
 3 files changed, 64 insertions(+), 9 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 97c11d557052..7119a34fef5c 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -2107,20 +2107,25 @@ static void __dasd_device_start_head(struct dasd_device *device)
 
 static void __dasd_device_check_path_events(struct dasd_device *device)
 {
+	__u8 tbvpm, fcsecpm;
 	int rc;
 
-	if (!dasd_path_get_tbvpm(device))
+	tbvpm = dasd_path_get_tbvpm(device);
+	fcsecpm = dasd_path_get_fcsecpm(device);
+
+	if (!tbvpm && !fcsecpm)
 		return;
 
 	if (device->stopped &
 	    ~(DASD_STOPPED_DC_WAIT | DASD_UNRESUMED_PM))
 		return;
-	rc = device->discipline->pe_handler(device,
-					    dasd_path_get_tbvpm(device));
-	if (rc)
+	rc = device->discipline->pe_handler(device, tbvpm, fcsecpm);
+	if (rc) {
 		dasd_device_set_timer(device, 50);
-	else
+	} else {
 		dasd_path_clear_all_verify(device);
+		dasd_path_clear_all_fcsec(device);
+	}
 };
 
 /*
@@ -3868,6 +3873,10 @@ void dasd_generic_path_event(struct ccw_device *cdev, int *path_event)
 			if (device->discipline->kick_validate)
 				device->discipline->kick_validate(device);
 		}
+		if (path_event[chp] & PE_PATH_FCES_EVENT) {
+			dasd_path_fcsec_update(device, chp);
+			dasd_schedule_device_bh(device);
+		}
 	}
 	hpfpm = dasd_path_get_hpfpm(device);
 	ifccpm = dasd_path_get_ifccpm(device);
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 822a8ad4e338..40443f26a742 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -111,6 +111,7 @@ struct pe_handler_work_data {
 	__u8 rcd_buffer[DASD_ECKD_RCD_DATA_SIZE];
 	int isglobal;
 	__u8 tbvpm;
+	__u8 fcsecpm;
 };
 static struct pe_handler_work_data *pe_handler_worker;
 static DEFINE_MUTEX(dasd_pe_handler_mutex);
@@ -1461,7 +1462,10 @@ static void do_pe_handler_work(struct work_struct *work)
 		return;
 	}
 
-	dasd_eckd_path_available_action(device, data);
+	if (data->tbvpm)
+		dasd_eckd_path_available_action(device, data);
+	if (data->fcsecpm)
+		dasd_eckd_read_fc_security(device);
 
 	clear_bit(DASD_FLAG_PATH_VERIFY, &device->flags);
 	dasd_put_device(device);
@@ -1471,7 +1475,8 @@ static void do_pe_handler_work(struct work_struct *work)
 		kfree(data);
 }
 
-static int dasd_eckd_pe_handler(struct dasd_device *device, __u8 lpm)
+static int dasd_eckd_pe_handler(struct dasd_device *device,
+				__u8 tbvpm, __u8 fcsecpm)
 {
 	struct pe_handler_work_data *data;
 
@@ -1490,7 +1495,8 @@ static int dasd_eckd_pe_handler(struct dasd_device *device, __u8 lpm)
 	INIT_WORK(&data->worker, do_pe_handler_work);
 	dasd_get_device(device);
 	data->device = device;
-	data->tbvpm = lpm;
+	data->tbvpm = tbvpm;
+	data->fcsecpm = fcsecpm;
 	schedule_work(&data->worker);
 	return 0;
 }
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index 3ae1c8d42b8a..8486ed3c877b 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -298,7 +298,7 @@ struct dasd_discipline {
 	 * configuration.
 	 */
 	int (*verify_path)(struct dasd_device *, __u8);
-	int (*pe_handler)(struct dasd_device *, __u8);
+	int (*pe_handler)(struct dasd_device *, __u8, __u8);
 
 	/*
 	 * Last things to do when a device is set online, and first things
@@ -423,6 +423,7 @@ extern struct dasd_discipline *dasd_diag_discipline_pointer;
 #define DASD_PATH_NOHPF        6
 #define DASD_PATH_CUIR	       7
 #define DASD_PATH_IFCC	       8
+#define DASD_PATH_FCSEC	       9
 
 #define DASD_THRHLD_MAX		4294967295U
 #define DASD_INTERVAL_MAX	4294967295U
@@ -965,6 +966,29 @@ static inline void dasd_path_clear_all_verify(struct dasd_device *device)
 		dasd_path_clear_verify(device, chp);
 }
 
+static inline void dasd_path_fcsec(struct dasd_device *device, int chp)
+{
+	__set_bit(DASD_PATH_FCSEC, &device->path[chp].flags);
+}
+
+static inline void dasd_path_clear_fcsec(struct dasd_device *device, int chp)
+{
+	__clear_bit(DASD_PATH_FCSEC, &device->path[chp].flags);
+}
+
+static inline int dasd_path_need_fcsec(struct dasd_device *device, int chp)
+{
+	return test_bit(DASD_PATH_FCSEC, &device->path[chp].flags);
+}
+
+static inline void dasd_path_clear_all_fcsec(struct dasd_device *device)
+{
+	int chp;
+
+	for (chp = 0; chp < 8; chp++)
+		dasd_path_clear_fcsec(device, chp);
+}
+
 static inline void dasd_path_operational(struct dasd_device *device, int chp)
 {
 	__set_bit(DASD_PATH_OPERATIONAL, &device->path[chp].flags);
@@ -1090,6 +1114,17 @@ static inline __u8 dasd_path_get_tbvpm(struct dasd_device *device)
 	return tbvpm;
 }
 
+static inline int dasd_path_get_fcsecpm(struct dasd_device *device)
+{
+	int chp;
+
+	for (chp = 0; chp < 8; chp++)
+		if (dasd_path_need_fcsec(device, chp))
+			return 1;
+
+	return 0;
+}
+
 static inline __u8 dasd_path_get_nppm(struct dasd_device *device)
 {
 	int chp;
@@ -1347,6 +1382,11 @@ static inline void dasd_path_notoper(struct dasd_device *device, int chp)
 	dasd_path_clear_nonpreferred(device, chp);
 }
 
+static inline void dasd_path_fcsec_update(struct dasd_device *device, int chp)
+{
+	dasd_path_fcsec(device, chp);
+}
+
 /*
  * remove all paths from normal operation
  */
-- 
2.17.1

Re: [PATCH 01/10] s390/cio: Export information about Endpoint-Security Capability

[Cornelia Huck]

On Fri,  2 Oct 2020 21:39:31 +0200
Stefan Haberland <sth@linux.ibm.com> wrote:

> From: Sebastian Ott <sebott@linux.ibm.com>
> 
> Add a new sysfs attribute 'esc' per chpid. This new attribute exports
> the Endpoint-Security-Capability byte of channel-path description block,
> which could be 0-None, 1-Authentication, 2 and 3-Encryption.
> 
> For example:
> $ cat /sys/devices/css0/chp0.34/esc
> 0
> 
> Reference-ID: IO1812
> Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
> [vneethv@linux.ibm.com: cleaned-up & modified description]
> Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
> Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> Acked-by: Vasily Gorbik <gor@linux.ibm.com>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
> ---
>  drivers/s390/cio/chp.c  | 15 +++++++++++++++
>  drivers/s390/cio/chsc.h |  3 ++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
> index dfcbe54591fb..8d0de6adcad0 100644
> --- a/drivers/s390/cio/chp.c
> +++ b/drivers/s390/cio/chp.c
> @@ -384,6 +384,20 @@ static ssize_t chp_chid_external_show(struct device *dev,
>  }
>  static DEVICE_ATTR(chid_external, 0444, chp_chid_external_show, NULL);
>  
> +static ssize_t chp_esc_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct channel_path *chp = to_channelpath(dev);
> +	ssize_t rc;
> +
> +	mutex_lock(&chp->lock);
> +	rc = sprintf(buf, "%x\n", chp->desc_fmt1.esc);

I'm wondering: Do we need to distinguish between '0' == 'no esc, and
the hardware says so' and '0' == 'the chsc to get that information is
not supported'? I see that for the chid the code checks for a flag in
desc_fmt1, and I indeed see that nothing is displayed for
chid/chid_external when I run under QEMU.

> +	mutex_unlock(&chp->lock);
> +
> +	return rc;
> +}
> +static DEVICE_ATTR(esc, 0444, chp_esc_show, NULL);
> +
>  static ssize_t util_string_read(struct file *filp, struct kobject *kobj,
>  				struct bin_attribute *attr, char *buf,
>  				loff_t off, size_t count)

(...)

Re: [PATCH 08/10] s390/dasd: Display FC Endpoint Security information via sysfs

[Cornelia Huck]

On Fri,  2 Oct 2020 21:39:38 +0200
Stefan Haberland <sth@linux.ibm.com> wrote:

> From: Jan Höppner <hoeppner@linux.ibm.com>
> 
> Add a new sysfs attribute (fc_security) per device and per operational
> channel path. The information of the current FC Endpoint Security state
> is received through the CIO layer.
> 
> The state of the FC Endpoint Security can be either "Unsupported",
> "Authentication", or "Encryption".
> 
> For example:
> $ cat /sys/bus/ccw/devices/0.0.c600/fc_security
> Encryption
> 
> If any of the operational paths is in a state different from all
> others, the device sysfs attribute will display the additional state
> "Inconsistent".
> 
> The sysfs attributes per paths are organised in a new directory called
> "paths_info" with subdirectories for each path.
> 
> /sys/bus/ccw/devices/0.0.c600/paths_info/
> ├── 0.38
> │   └── fc_security
> ├── 0.39
> │   └── fc_security
> ├── 0.3a
> │   └── fc_security
> └── 0.3b
>     └── fc_security
> 
> Reference-ID: IO1812
> Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
> Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
> ---
>  drivers/s390/block/dasd_devmap.c | 105 +++++++++++++++++++++++++++++++
>  drivers/s390/block/dasd_eckd.c   |  30 +++++++++
>  drivers/s390/block/dasd_int.h    |  68 ++++++++++++++++++++
>  3 files changed, 203 insertions(+)
> 

(...)

> +static struct kobj_type path_attr_type = {
> +	.release	= dasd_path_release,

This function does nothing; I think there's something wrong with your
kobject handling?

> +	.default_attrs	= paths_info_attrs,
> +	.sysfs_ops	= &kobj_sysfs_ops,
> +};
> +
> +static void dasd_path_init_kobj(struct dasd_device *device, int chp)
> +{
> +	device->path[chp].kobj.kset = device->paths_info;
> +	kobject_init(&device->path[chp].kobj, &path_attr_type);

This inits a static kobject; as you never free it, doesn't the code
moan about state_initialized if you try to do that a second time?

> +}
> +
> +void dasd_path_create_kobj(struct dasd_device *device, int chp)
> +{
> +	int rc;
> +
> +	if (test_bit(DASD_FLAG_OFFLINE, &device->flags))
> +		return;
> +	if (!device->paths_info) {
> +		dev_warn(&device->cdev->dev, "Unable to create paths objects\n");

I guess this warns every time you come along here, is warning more than
once useful?

> +		return;
> +	}
> +	if (device->path[chp].in_sysfs)
> +		return;
> +	if (!device->path[chp].conf_data)

Out of interest: Have you tried this with vfio-ccw under QEMU, where
some information is simply not available?

> +		return;
> +
> +	dasd_path_init_kobj(device, chp);
> +
> +	rc = kobject_add(&device->path[chp].kobj, NULL, "%x.%02x",
> +			 device->path[chp].cssid, device->path[chp].chpid);
> +	if (rc)
> +		kobject_put(&device->path[chp].kobj);

This will eventually lead to the nop release function, which doesn't
unset state_initialized (see above) -- but OTOH, it shouldn't muck
around with kobject internals anyway.

I think the kobjects really want to be dynamically allocated; instead
of going through a remove/add cycle, is there a way to make path
objects "invisible" instead? Or add an "available" attribute, and error
out reading any other attribute?

> +	device->path[chp].in_sysfs = true;
> +}
> +EXPORT_SYMBOL(dasd_path_create_kobj);
> +
> +void dasd_path_create_kobjects(struct dasd_device *device)
> +{
> +	u8 lpm, opm;
> +
> +	opm = dasd_path_get_opm(device);
> +	for (lpm = 0x80; lpm; lpm >>= 1) {
> +		if (!(lpm & opm))
> +			continue;

Any reason you do not simply create objects for _all_ paths, combined
with returning n/a or erroring out for paths where this does not apply?
(I might be missing something obvious.)

> +		dasd_path_create_kobj(device, pathmask_to_pos(lpm));
> +	}
> +}
> +EXPORT_SYMBOL(dasd_path_create_kobjects);
> +
> +void dasd_path_remove_kobj(struct dasd_device *device, int chp)
> +{
> +	if (device->path[chp].in_sysfs) {
> +		kobject_put(&device->path[chp].kobj);
> +		device->path[chp].in_sysfs = false;
> +	}
> +}
> +EXPORT_SYMBOL(dasd_path_remove_kobj);

Also, how is userspace supposed to deal with changes here? Should there
be a uevent on the parent device to notify about changes?

>  
>  int dasd_add_sysfs_files(struct ccw_device *cdev)
>  {

(...)

> +static inline void dasd_path_release(struct kobject *kobj)
> +{
> +/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
> +}
> +

As already said, I don't think that's a correct way to implement this.

(...)

Re: [PATCH 01/10] s390/cio: Export information about Endpoint-Security Capability

[Stefan Haberland]

Hi,

talked to Vineeth, here is his answer...

Am 06.10.20 um 11:46 schrieb Cornelia Huck:
> On Fri,  2 Oct 2020 21:39:31 +0200
> Stefan Haberland <sth@linux.ibm.com> wrote:
>
>> From: Sebastian Ott <sebott@linux.ibm.com>
>>
>> Add a new sysfs attribute 'esc' per chpid. This new attribute exports
>> the Endpoint-Security-Capability byte of channel-path description block,
>> which could be 0-None, 1-Authentication, 2 and 3-Encryption.
>>
>> For example:
>> $ cat /sys/devices/css0/chp0.34/esc
>> 0
>>
>> Reference-ID: IO1812
>> Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
>> [vneethv@linux.ibm.com: cleaned-up & modified description]
>> Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
>> Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
>> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
>> Acked-by: Vasily Gorbik <gor@linux.ibm.com>
>> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
>> ---
>>  drivers/s390/cio/chp.c  | 15 +++++++++++++++
>>  drivers/s390/cio/chsc.h |  3 ++-
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
>> index dfcbe54591fb..8d0de6adcad0 100644
>> --- a/drivers/s390/cio/chp.c
>> +++ b/drivers/s390/cio/chp.c
>> @@ -384,6 +384,20 @@ static ssize_t chp_chid_external_show(struct device *dev,
>>  }
>>  static DEVICE_ATTR(chid_external, 0444, chp_chid_external_show, NULL);
>>  
>> +static ssize_t chp_esc_show(struct device *dev,
>> +			    struct device_attribute *attr, char *buf)
>> +{
>> +	struct channel_path *chp = to_channelpath(dev);
>> +	ssize_t rc;
>> +
>> +	mutex_lock(&chp->lock);
>> +	rc = sprintf(buf, "%x\n", chp->desc_fmt1.esc);
> I'm wondering: Do we need to distinguish between '0' == 'no esc, and
> the hardware says so' and '0' == 'the chsc to get that information is
> not supported'? I see that for the chid the code checks for a flag in
> desc_fmt1, and I indeed see that nothing is displayed for
> chid/chid_external when I run under QEMU.

ESC==0 due to 'missing support for the required CHSC information' is
just another symptom of "unsupported" because the CSS firmware code
doesn't bring the required support.
Also, not sure if there is any flag/value which provide this
distinction. So we think having 2 different values "Unknown" and
"Unsupported" is not required in this scenario.

So, we kept a single "ESC==0" which indicates "Unsupported", but as you
mentioned, in different levels.

>> +	mutex_unlock(&chp->lock);
>> +
>> +	return rc;
>> +}
>> +static DEVICE_ATTR(esc, 0444, chp_esc_show, NULL);
>> +
>>  static ssize_t util_string_read(struct file *filp, struct kobject *kobj,
>>  				struct bin_attribute *attr, char *buf,
>>  				loff_t off, size_t count)
> (...)
>

Re: [PATCH 01/10] s390/cio: Export information about Endpoint-Security Capability

[Cornelia Huck]

On Tue, 6 Oct 2020 16:23:36 +0200
Stefan Haberland <sth@linux.ibm.com> wrote:

> Hi,
> 
> talked to Vineeth, here is his answer...
> 
> Am 06.10.20 um 11:46 schrieb Cornelia Huck:
> > On Fri,  2 Oct 2020 21:39:31 +0200
> > Stefan Haberland <sth@linux.ibm.com> wrote:
> >  
> >> From: Sebastian Ott <sebott@linux.ibm.com>
> >>
> >> Add a new sysfs attribute 'esc' per chpid. This new attribute exports
> >> the Endpoint-Security-Capability byte of channel-path description block,
> >> which could be 0-None, 1-Authentication, 2 and 3-Encryption.
> >>
> >> For example:
> >> $ cat /sys/devices/css0/chp0.34/esc
> >> 0
> >>
> >> Reference-ID: IO1812
> >> Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
> >> [vneethv@linux.ibm.com: cleaned-up & modified description]
> >> Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
> >> Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
> >> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> >> Acked-by: Vasily Gorbik <gor@linux.ibm.com>
> >> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
> >> ---
> >>  drivers/s390/cio/chp.c  | 15 +++++++++++++++
> >>  drivers/s390/cio/chsc.h |  3 ++-
> >>  2 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
> >> index dfcbe54591fb..8d0de6adcad0 100644
> >> --- a/drivers/s390/cio/chp.c
> >> +++ b/drivers/s390/cio/chp.c
> >> @@ -384,6 +384,20 @@ static ssize_t chp_chid_external_show(struct device *dev,
> >>  }
> >>  static DEVICE_ATTR(chid_external, 0444, chp_chid_external_show, NULL);
> >>  
> >> +static ssize_t chp_esc_show(struct device *dev,
> >> +			    struct device_attribute *attr, char *buf)
> >> +{
> >> +	struct channel_path *chp = to_channelpath(dev);
> >> +	ssize_t rc;
> >> +
> >> +	mutex_lock(&chp->lock);
> >> +	rc = sprintf(buf, "%x\n", chp->desc_fmt1.esc);  
> > I'm wondering: Do we need to distinguish between '0' == 'no esc, and
> > the hardware says so' and '0' == 'the chsc to get that information is
> > not supported'? I see that for the chid the code checks for a flag in
> > desc_fmt1, and I indeed see that nothing is displayed for
> > chid/chid_external when I run under QEMU.  
> 
> ESC==0 due to 'missing support for the required CHSC information' is
> just another symptom of "unsupported" because the CSS firmware code
> doesn't bring the required support.
> Also, not sure if there is any flag/value which provide this
> distinction. So we think having 2 different values "Unknown" and
> "Unsupported" is not required in this scenario.
> 
> So, we kept a single "ESC==0" which indicates "Unsupported", but as you
> mentioned, in different levels.

Ok, that makes sense, also considering how this is used later on.

> 
> >> +	mutex_unlock(&chp->lock);
> >> +
> >> +	return rc;
> >> +}
> >> +static DEVICE_ATTR(esc, 0444, chp_esc_show, NULL);
> >> +
> >>  static ssize_t util_string_read(struct file *filp, struct kobject *kobj,
> >>  				struct bin_attribute *attr, char *buf,
> >>  				loff_t off, size_t count)  
> > (...)
> >  
> 
> 

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 02/10] s390/cio: Provide Endpoint-Security Mode per CU

[Cornelia Huck]

On Fri,  2 Oct 2020 21:39:32 +0200
Stefan Haberland <sth@linux.ibm.com> wrote:

> From: Vineeth Vijayan <vneethv@linux.ibm.com>
> 
> Add an interface in the CIO layer to retrieve the information about the
> Endpoint-Security Mode (ESM) of the specified CU. The ESM values are
> defined as 0-None, 1-Authenticated or 2, 3-Encrypted.
> 
> Reference-ID: IO1812
> Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
> [vneethv@linux.ibm.com: cleaned-up and modified description]
> Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> Acked-by: Vasily Gorbik <gor@linux.ibm.com>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
> ---
>  arch/s390/include/asm/cio.h |  1 +
>  drivers/s390/cio/chsc.c     | 83 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
 
(...)

> +/**
> + * chsc_scud() - Store control-unit description.
> + * @cu:		number of the control-unit
> + * @esm:	8 1-byte endpoint security mode values
> + * @esm_valid:	validity mask for @esm
> + *
> + * Interface to retrieve information about the endpoint security
> + * modes for up to 8 paths of a control unit.
> + *
> + * Returns 0 on success.
> + */
> +int chsc_scud(u16 cu, u64 *esm, u8 *esm_valid)
> +{
> +	struct chsc_scud *scud = chsc_page;
> +	int ret;
> +

I'm wondering if it would make sense to check in the chsc
characteristics whether that chsc is actually installed (if there's
actually a bit for it, although I'd expect so). Some existing chscs
check for bits in the characteristics, others don't. (Don't know
whether QEMU is the only platform that doesn't provide this chsc.)

> +	spin_lock_irq(&chsc_page_lock);
> +	memset(chsc_page, 0, PAGE_SIZE);
> +	scud->request.length = SCUD_REQ_LEN;
> +	scud->request.code = SCUD_REQ_CMD;
> +	scud->fmt = 0;
> +	scud->cssid = 0;
> +	scud->first_cu = cu;
> +	scud->last_cu = cu;
> +
> +	ret = chsc(scud);
> +	if (!ret)
> +		ret = chsc_error_from_response(scud->response.code);
> +
> +	if (!ret && (scud->response.length <= 8 || scud->fmt_resp != 0
> +			|| !(scud->cudb[0].flags & 0x80)
> +			|| scud->cudb[0].cu != cu)) {
> +
> +		CIO_MSG_EVENT(2, "chsc: scud failed rc=%04x, L2=%04x "
> +			"FMT=%04x, cudb.flags=%02x, cudb.cu=%04x",
> +			scud->response.code, scud->response.length,
> +			scud->fmt_resp, scud->cudb[0].flags, scud->cudb[0].cu);
> +		ret = -EINVAL;
> +	}
> +
> +	if (ret)
> +		goto out;
> +
> +	memcpy(esm, scud->cudb[0].esm, sizeof(*esm));
> +	*esm_valid = scud->cudb[0].esm_valid;
> +out:
> +	spin_unlock_irq(&chsc_page_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(chsc_scud);

Re: [PATCH 08/10] s390/dasd: Display FC Endpoint Security information via sysfs

[Jan Höppner]

On 10/6/20 12:26 PM, Cornelia Huck wrote:
> On Fri,  2 Oct 2020 21:39:38 +0200
> Stefan Haberland <sth@linux.ibm.com> wrote:
> 
>> From: Jan Höppner <hoeppner@linux.ibm.com>
>>
>> Add a new sysfs attribute (fc_security) per device and per operational
>> channel path. The information of the current FC Endpoint Security state
>> is received through the CIO layer.
>>
>> The state of the FC Endpoint Security can be either "Unsupported",
>> "Authentication", or "Encryption".
>>
>> For example:
>> $ cat /sys/bus/ccw/devices/0.0.c600/fc_security
>> Encryption
>>
>> If any of the operational paths is in a state different from all
>> others, the device sysfs attribute will display the additional state
>> "Inconsistent".
>>
>> The sysfs attributes per paths are organised in a new directory called
>> "paths_info" with subdirectories for each path.
>>
>> /sys/bus/ccw/devices/0.0.c600/paths_info/
>> ├── 0.38
>> │   └── fc_security
>> ├── 0.39
>> │   └── fc_security
>> ├── 0.3a
>> │   └── fc_security
>> └── 0.3b
>>     └── fc_security
>>
>> Reference-ID: IO1812
>> Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
>> Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
>> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
>> ---
>>  drivers/s390/block/dasd_devmap.c | 105 +++++++++++++++++++++++++++++++
>>  drivers/s390/block/dasd_eckd.c   |  30 +++++++++
>>  drivers/s390/block/dasd_int.h    |  68 ++++++++++++++++++++
>>  3 files changed, 203 insertions(+)
>>
> 
> (...)
> 
>> +static struct kobj_type path_attr_type = {
>> +	.release	= dasd_path_release,
> 
> This function does nothing; I think there's something wrong with your
> kobject handling?

Explanation below.

> 
>> +	.default_attrs	= paths_info_attrs,
>> +	.sysfs_ops	= &kobj_sysfs_ops,
>> +};
>> +
>> +static void dasd_path_init_kobj(struct dasd_device *device, int chp)
>> +{
>> +	device->path[chp].kobj.kset = device->paths_info;
>> +	kobject_init(&device->path[chp].kobj, &path_attr_type);
> 
> This inits a static kobject; as you never free it, doesn't the code

kobject_put() frees the kobject data.

> moan about state_initialized if you try to do that a second time?

No, because we check whether we have this kobject already present
in sysfs before we try to initialize it (we have in_sysfs per path
object for this).

> 
>> +}
>> +
>> +void dasd_path_create_kobj(struct dasd_device *device, int chp)
>> +{
>> +	int rc;
>> +
>> +	if (test_bit(DASD_FLAG_OFFLINE, &device->flags))
>> +		return;
>> +	if (!device->paths_info) {
>> +		dev_warn(&device->cdev->dev, "Unable to create paths objects\n");
> 
> I guess this warns every time you come along here, is warning more than
> once useful?
> 

paths_info is a kset created during the device initialization. Do you mean,
in case the kset creation fails, this check here should only warn once?
I'm not sure about that, hm.

>> +		return;
>> +	}
>> +	if (device->path[chp].in_sysfs)
>> +		return;
>> +	if (!device->path[chp].conf_data)
> 
> Out of interest: Have you tried this with vfio-ccw under QEMU, where
> some information is simply not available?

I did not, sorry.

> 
>> +		return;
>> +
>> +	dasd_path_init_kobj(device, chp);
>> +
>> +	rc = kobject_add(&device->path[chp].kobj, NULL, "%x.%02x",
>> +			 device->path[chp].cssid, device->path[chp].chpid);
>> +	if (rc)
>> +		kobject_put(&device->path[chp].kobj);
> 
> This will eventually lead to the nop release function, which doesn't
> unset state_initialized (see above) -- but OTOH, it shouldn't muck
> around with kobject internals anyway.

The release function is supposed to free memory of the structure where
the kobject lies in (our release function is explained below).
The rest is taking care of by the kobject library.

> 
> I think the kobjects really want to be dynamically allocated; instead
> of going through a remove/add cycle, is there a way to make path
> objects "invisible" instead? Or add an "available" attribute, and error
> out reading any other attribute?
> 
>> +	device->path[chp].in_sysfs = true;
>> +}
>> +EXPORT_SYMBOL(dasd_path_create_kobj);
>> +
>> +void dasd_path_create_kobjects(struct dasd_device *device)
>> +{
>> +	u8 lpm, opm;
>> +
>> +	opm = dasd_path_get_opm(device);
>> +	for (lpm = 0x80; lpm; lpm >>= 1) {
>> +		if (!(lpm & opm))
>> +			continue;
> 
> Any reason you do not simply create objects for _all_ paths, combined
> with returning n/a or erroring out for paths where this does not apply?
> (I might be missing something obvious.)

Because we likely don't have all information required to create the kobject
for other paths, e.g. the cssid and chpid (which are required for the
proper name).

> 
>> +		dasd_path_create_kobj(device, pathmask_to_pos(lpm));
>> +	}
>> +}
>> +EXPORT_SYMBOL(dasd_path_create_kobjects);
>> +
>> +void dasd_path_remove_kobj(struct dasd_device *device, int chp)
>> +{
>> +	if (device->path[chp].in_sysfs) {
>> +		kobject_put(&device->path[chp].kobj);
>> +		device->path[chp].in_sysfs = false;
>> +	}
>> +}
>> +EXPORT_SYMBOL(dasd_path_remove_kobj);
> 
> Also, how is userspace supposed to deal with changes here? Should there
> be a uevent on the parent device to notify about changes?

I can't think of a user just yet. I'll keep this in mind for further
improvements. I certainly won't hurt to create uevents here.

> 
>>  
>>  int dasd_add_sysfs_files(struct ccw_device *cdev)
>>  {
> 
> (...)
> 
>> +static inline void dasd_path_release(struct kobject *kobj)
>> +{
>> +/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
>> +}
>> +
> 
> As already said, I don't think that's a correct way to implement this.
> 

As you correctly pointed out, our release function doesn't do anything.
This is because our path data is a (static) part of our device.
This data is critical to keep our devices operational.
We can't simply rely on allocated memory if systems are under stress. 

Having this data dynamically allocated involves a lot of rework of our
path handling as well. There are a few things that are subject to improvement
and evaluating whether our dasd_path structures can be dynamic is one of
these things. However, even then, the above concern persists and I
highly doubt that dynamic dasd_paths objects are doable for us at this
moment.

I do understand the concerns, however, we release the memory for dasd_path
structures eventually when dasd_free_device() is called. Until that point,
the data has to be kept alive. The rest is taking care of by the kobject
library.
In our path handling we also make sure that we can always verify/validate
paths information even if a system is under high memory pressure. Another
reason why it would contradictory for dasd_path objects to be dynamic.

I hope this explains the reasoning behind the release function.

so long,
Jan

Re: [PATCH 08/10] s390/dasd: Display FC Endpoint Security information via sysfs

[Cornelia Huck]

On Tue, 6 Oct 2020 18:57:46 +0200
Jan Höppner <hoeppner@linux.ibm.com> wrote:

> On 10/6/20 12:26 PM, Cornelia Huck wrote:
> > On Fri,  2 Oct 2020 21:39:38 +0200
> > Stefan Haberland <sth@linux.ibm.com> wrote:
> >   
> >> From: Jan Höppner <hoeppner@linux.ibm.com>
> >>
> >> Add a new sysfs attribute (fc_security) per device and per operational
> >> channel path. The information of the current FC Endpoint Security state
> >> is received through the CIO layer.
> >>
> >> The state of the FC Endpoint Security can be either "Unsupported",
> >> "Authentication", or "Encryption".
> >>
> >> For example:
> >> $ cat /sys/bus/ccw/devices/0.0.c600/fc_security
> >> Encryption
> >>
> >> If any of the operational paths is in a state different from all
> >> others, the device sysfs attribute will display the additional state
> >> "Inconsistent".
> >>
> >> The sysfs attributes per paths are organised in a new directory called
> >> "paths_info" with subdirectories for each path.
> >>
> >> /sys/bus/ccw/devices/0.0.c600/paths_info/
> >> ├── 0.38
> >> │   └── fc_security
> >> ├── 0.39
> >> │   └── fc_security
> >> ├── 0.3a
> >> │   └── fc_security
> >> └── 0.3b
> >>     └── fc_security
> >>
> >> Reference-ID: IO1812
> >> Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
> >> Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
> >> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
> >> ---
> >>  drivers/s390/block/dasd_devmap.c | 105 +++++++++++++++++++++++++++++++
> >>  drivers/s390/block/dasd_eckd.c   |  30 +++++++++
> >>  drivers/s390/block/dasd_int.h    |  68 ++++++++++++++++++++
> >>  3 files changed, 203 insertions(+)
> >>  
> > 
> > (...)
> >   
> >> +static struct kobj_type path_attr_type = {
> >> +	.release	= dasd_path_release,  
> > 
> > This function does nothing; I think there's something wrong with your
> > kobject handling?  
> 
> Explanation below.
> 
> >   
> >> +	.default_attrs	= paths_info_attrs,
> >> +	.sysfs_ops	= &kobj_sysfs_ops,
> >> +};
> >> +
> >> +static void dasd_path_init_kobj(struct dasd_device *device, int chp)
> >> +{
> >> +	device->path[chp].kobj.kset = device->paths_info;
> >> +	kobject_init(&device->path[chp].kobj, &path_attr_type);  
> > 
> > This inits a static kobject; as you never free it, doesn't the code  
> 
> kobject_put() frees the kobject data.

Not quite; if the last ref is put, it invokes the provided ->release
callback and frees the allocated name. If the ->release callback does
not free the object embedding the kobject, only the name is freed
AFAICS.

> 
> > moan about state_initialized if you try to do that a second time?  
> 
> No, because we check whether we have this kobject already present
> in sysfs before we try to initialize it (we have in_sysfs per path
> object for this).

I might be confused by the path revalidation logic; but don't you unset
in_sysfs when you remove the path object? What happens when you readd it?

> 
> >   
> >> +}
> >> +
> >> +void dasd_path_create_kobj(struct dasd_device *device, int chp)
> >> +{
> >> +	int rc;
> >> +
> >> +	if (test_bit(DASD_FLAG_OFFLINE, &device->flags))
> >> +		return;
> >> +	if (!device->paths_info) {
> >> +		dev_warn(&device->cdev->dev, "Unable to create paths objects\n");  
> > 
> > I guess this warns every time you come along here, is warning more than
> > once useful?
> >   
> 
> paths_info is a kset created during the device initialization. Do you mean,
> in case the kset creation fails, this check here should only warn once?
> I'm not sure about that, hm.

If the kset could not be set up during init, you'll hit this for every
path object you want to add afterwards -- one warning per device of
that should be enough, I guess :)

> 
> >> +		return;
> >> +	}
> >> +	if (device->path[chp].in_sysfs)
> >> +		return;
> >> +	if (!device->path[chp].conf_data)  
> > 
> > Out of interest: Have you tried this with vfio-ccw under QEMU, where
> > some information is simply not available?  
> 
> I did not, sorry.
> 
> >   
> >> +		return;
> >> +
> >> +	dasd_path_init_kobj(device, chp);
> >> +
> >> +	rc = kobject_add(&device->path[chp].kobj, NULL, "%x.%02x",
> >> +			 device->path[chp].cssid, device->path[chp].chpid);
> >> +	if (rc)
> >> +		kobject_put(&device->path[chp].kobj);  
> > 
> > This will eventually lead to the nop release function, which doesn't
> > unset state_initialized (see above) -- but OTOH, it shouldn't muck
> > around with kobject internals anyway.  
> 
> The release function is supposed to free memory of the structure where
> the kobject lies in (our release function is explained below).
> The rest is taking care of by the kobject library.

Yes, but the kobject code does not unset state_initialized.
> 
> > 
> > I think the kobjects really want to be dynamically allocated; instead
> > of going through a remove/add cycle, is there a way to make path
> > objects "invisible" instead? Or add an "available" attribute, and error
> > out reading any other attribute?

> > (...)
> >   
> >> +static inline void dasd_path_release(struct kobject *kobj)
> >> +{
> >> +/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
> >> +}
> >> +  
> > 
> > As already said, I don't think that's a correct way to implement this.
> >   
> 
> As you correctly pointed out, our release function doesn't do anything.
> This is because our path data is a (static) part of our device.
> This data is critical to keep our devices operational.
> We can't simply rely on allocated memory if systems are under stress. 

Yes, avoiding freeing and reallocating memory certainly makes sense.

> 
> Having this data dynamically allocated involves a lot of rework of our
> path handling as well. There are a few things that are subject to improvement
> and evaluating whether our dasd_path structures can be dynamic is one of
> these things. However, even then, the above concern persists and I
> highly doubt that dynamic dasd_paths objects are doable for us at this
> moment.
> 
> I do understand the concerns, however, we release the memory for dasd_path
> structures eventually when dasd_free_device() is called. Until that point,
> the data has to be kept alive. The rest is taking care of by the kobject
> library.

Yes, there doesn't seem to be any memory leakage.

> In our path handling we also make sure that we can always verify/validate
> paths information even if a system is under high memory pressure. Another
> reason why it would contradictory for dasd_path objects to be dynamic.
> 
> I hope this explains the reasoning behind the release function.

I understand where you're coming from.

However, "static" kobjects (in the sense of "we may re-register the
same kobject") are still problematic. Is there any way to simply
"disappear" path objects that are not valid at the moment, or mark them
as not valid?

Also, the simple act of registering/unregistering a kobject already
creates stress from its sysfs interactions... it seems you should try
to avoid that as well?

Re: [PATCH 02/10] s390/cio: Provide Endpoint-Security Mode per CU

[Stefan Haberland]

Am 06.10.20 um 16:46 schrieb Cornelia Huck:
> On Fri,  2 Oct 2020 21:39:32 +0200
> Stefan Haberland <sth@linux.ibm.com> wrote:
>
>> From: Vineeth Vijayan <vneethv@linux.ibm.com>
>>
>> Add an interface in the CIO layer to retrieve the information about the
>> Endpoint-Security Mode (ESM) of the specified CU. The ESM values are
>> defined as 0-None, 1-Authenticated or 2, 3-Encrypted.
>>
>> Reference-ID: IO1812
>> Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
>> [vneethv@linux.ibm.com: cleaned-up and modified description]
>> Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
>> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
>> Acked-by: Vasily Gorbik <gor@linux.ibm.com>
>> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
>> ---
>>  arch/s390/include/asm/cio.h |  1 +
>>  drivers/s390/cio/chsc.c     | 83 +++++++++++++++++++++++++++++++++++++
>>  2 files changed, 84 insertions(+)
>  
> (...)
>
>> +/**
>> + * chsc_scud() - Store control-unit description.
>> + * @cu:		number of the control-unit
>> + * @esm:	8 1-byte endpoint security mode values
>> + * @esm_valid:	validity mask for @esm
>> + *
>> + * Interface to retrieve information about the endpoint security
>> + * modes for up to 8 paths of a control unit.
>> + *
>> + * Returns 0 on success.
>> + */
>> +int chsc_scud(u16 cu, u64 *esm, u8 *esm_valid)
>> +{
>> +	struct chsc_scud *scud = chsc_page;
>> +	int ret;
>> +
> I'm wondering if it would make sense to check in the chsc
> characteristics whether that chsc is actually installed (if there's
> actually a bit for it, although I'd expect so). Some existing chscs
> check for bits in the characteristics, others don't. (Don't know
> whether QEMU is the only platform that doesn't provide this chsc.)

I don't see any benefit in checking upfront if the CHSC is supported -
we'll get
a corresponding CHSC response code and since no error message is logged
for this
case, the outcome would be the same as if we checked for the
characteristics bit
beforehand.


>> +	spin_lock_irq(&chsc_page_lock);
>> +	memset(chsc_page, 0, PAGE_SIZE);
>> +	scud->request.length = SCUD_REQ_LEN;
>> +	scud->request.code = SCUD_REQ_CMD;
>> +	scud->fmt = 0;
>> +	scud->cssid = 0;
>> +	scud->first_cu = cu;
>> +	scud->last_cu = cu;
>> +
>> +	ret = chsc(scud);
>> +	if (!ret)
>> +		ret = chsc_error_from_response(scud->response.code);
>> +
>> +	if (!ret && (scud->response.length <= 8 || scud->fmt_resp != 0
>> +			|| !(scud->cudb[0].flags & 0x80)
>> +			|| scud->cudb[0].cu != cu)) {
>> +
>> +		CIO_MSG_EVENT(2, "chsc: scud failed rc=%04x, L2=%04x "
>> +			"FMT=%04x, cudb.flags=%02x, cudb.cu=%04x",
>> +			scud->response.code, scud->response.length,
>> +			scud->fmt_resp, scud->cudb[0].flags, scud->cudb[0].cu);
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	if (ret)
>> +		goto out;
>> +
>> +	memcpy(esm, scud->cudb[0].esm, sizeof(*esm));
>> +	*esm_valid = scud->cudb[0].esm_valid;
>> +out:
>> +	spin_unlock_irq(&chsc_page_lock);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(chsc_scud);

Re: [PATCH 08/10] s390/dasd: Display FC Endpoint Security information via sysfs

[Jan Höppner]

... snip ...
>>>   
>>>> +static struct kobj_type path_attr_type = {
>>>> +	.release	= dasd_path_release,  
>>>
>>> This function does nothing; I think there's something wrong with your
>>> kobject handling?  
>>
>> Explanation below.
>>
>>>   
>>>> +	.default_attrs	= paths_info_attrs,
>>>> +	.sysfs_ops	= &kobj_sysfs_ops,
>>>> +};
>>>> +
>>>> +static void dasd_path_init_kobj(struct dasd_device *device, int chp)
>>>> +{
>>>> +	device->path[chp].kobj.kset = device->paths_info;
>>>> +	kobject_init(&device->path[chp].kobj, &path_attr_type);  
>>>
>>> This inits a static kobject; as you never free it, doesn't the code  
>>
>> kobject_put() frees the kobject data.
> 
> Not quite; if the last ref is put, it invokes the provided ->release
> callback and frees the allocated name. If the ->release callback does
> not free the object embedding the kobject, only the name is freed
> AFAICS.
> 

True, the rest is freed when the device is being destroyed with
dasd_free_device().

>>
>>> moan about state_initialized if you try to do that a second time?  
>>
>> No, because we check whether we have this kobject already present
>> in sysfs before we try to initialize it (we have in_sysfs per path
>> object for this).
> 
> I might be confused by the path revalidation logic; but don't you unset
> in_sysfs when you remove the path object? What happens when you readd it?

We set it to true again (See dasd_path_create_kobj()). (More details below)

> 
>>
>>>   
>>>> +}
>>>> +
>>>> +void dasd_path_create_kobj(struct dasd_device *device, int chp)
>>>> +{
>>>> +	int rc;
>>>> +
>>>> +	if (test_bit(DASD_FLAG_OFFLINE, &device->flags))
>>>> +		return;
>>>> +	if (!device->paths_info) {
>>>> +		dev_warn(&device->cdev->dev, "Unable to create paths objects\n");  
>>>
>>> I guess this warns every time you come along here, is warning more than
>>> once useful?
>>>   
>>
>> paths_info is a kset created during the device initialization. Do you mean,
>> in case the kset creation fails, this check here should only warn once?
>> I'm not sure about that, hm.
> 
> If the kset could not be set up during init, you'll hit this for every
> path object you want to add afterwards -- one warning per device of
> that should be enough, I guess :)

I think this could be changed to one warning.

> 
>>
>>>> +		return;
>>>> +	}
>>>> +	if (device->path[chp].in_sysfs)
>>>> +		return;
>>>> +	if (!device->path[chp].conf_data)  
>>>
>>> Out of interest: Have you tried this with vfio-ccw under QEMU, where
>>> some information is simply not available?  
>>
>> I did not, sorry.
>>
>>>   
>>>> +		return;
>>>> +
>>>> +	dasd_path_init_kobj(device, chp);
>>>> +
>>>> +	rc = kobject_add(&device->path[chp].kobj, NULL, "%x.%02x",
>>>> +			 device->path[chp].cssid, device->path[chp].chpid);
>>>> +	if (rc)
>>>> +		kobject_put(&device->path[chp].kobj);  
>>>
>>> This will eventually lead to the nop release function, which doesn't
>>> unset state_initialized (see above) -- but OTOH, it shouldn't muck
>>> around with kobject internals anyway.  
>>
>> The release function is supposed to free memory of the structure where
>> the kobject lies in (our release function is explained below).
>> The rest is taking care of by the kobject library.
> 
> Yes, but the kobject code does not unset state_initialized.
>>
>>>
>>> I think the kobjects really want to be dynamically allocated; instead
>>> of going through a remove/add cycle, is there a way to make path
>>> objects "invisible" instead? Or add an "available" attribute, and error
>>> out reading any other attribute?
> 
>>> (...)
>>>   
>>>> +static inline void dasd_path_release(struct kobject *kobj)
>>>> +{
>>>> +/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
>>>> +}
>>>> +  
>>>
>>> As already said, I don't think that's a correct way to implement this.
>>>   
>>
>> As you correctly pointed out, our release function doesn't do anything.
>> This is because our path data is a (static) part of our device.
>> This data is critical to keep our devices operational.
>> We can't simply rely on allocated memory if systems are under stress. 
> 
> Yes, avoiding freeing and reallocating memory certainly makes sense.
> 
>>
>> Having this data dynamically allocated involves a lot of rework of our
>> path handling as well. There are a few things that are subject to improvement
>> and evaluating whether our dasd_path structures can be dynamic is one of
>> these things. However, even then, the above concern persists and I
>> highly doubt that dynamic dasd_paths objects are doable for us at this
>> moment.
>>
>> I do understand the concerns, however, we release the memory for dasd_path
>> structures eventually when dasd_free_device() is called. Until that point,
>> the data has to be kept alive. The rest is taking care of by the kobject
>> library.
> 
> Yes, there doesn't seem to be any memory leakage.
> 
>> In our path handling we also make sure that we can always verify/validate
>> paths information even if a system is under high memory pressure. Another
>> reason why it would contradictory for dasd_path objects to be dynamic.
>>
>> I hope this explains the reasoning behind the release function.
> 
> I understand where you're coming from.
> 
> However, "static" kobjects (in the sense of "we may re-register the
> same kobject") are still problematic. Is there any way to simply
> "disappear" path objects that are not valid at the moment, or mark them
> as not valid?

You could use kobject_del(), but it is rather intended to be used for
a two-stage removal of the kobject.

> 
> Also, the simple act of registering/unregistering a kobject already
> creates stress from its sysfs interactions... it seems you should try
> to avoid that as well?
> 

We don't re-register kobjects over and over again. The kobjects are
infact initialized and created only _once_. This is done either during
device initialization (after dasd_eckd_read_conf() in
dasd_eckd_check_characteristics()) or when a path is newly added
(in the path event handler).
The kobject will stay until the memory for the whole device is being
freed. This is also the reason why the kobject can stay initialized and
we track ourselves whether we did the initialization/creation already
(which we check e.g. when a path is removed and added again).
So, instead of the release function freeing the kobject data,
it is done by our dasd_free_device() (same thing, different function IMHO).

I think the concerns would be more worrisome if we'd remove/add
the kobjects every time. And then I agree, we'd run into trouble.

Re: [PATCH 02/10] s390/cio: Provide Endpoint-Security Mode per CU

[Cornelia Huck]

On Wed, 7 Oct 2020 16:24:06 +0200
Stefan Haberland <sth@linux.ibm.com> wrote:

> Am 06.10.20 um 16:46 schrieb Cornelia Huck:
> > On Fri,  2 Oct 2020 21:39:32 +0200
> > Stefan Haberland <sth@linux.ibm.com> wrote:
> >  
> >> From: Vineeth Vijayan <vneethv@linux.ibm.com>
> >>
> >> Add an interface in the CIO layer to retrieve the information about the
> >> Endpoint-Security Mode (ESM) of the specified CU. The ESM values are
> >> defined as 0-None, 1-Authenticated or 2, 3-Encrypted.
> >>
> >> Reference-ID: IO1812
> >> Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
> >> [vneethv@linux.ibm.com: cleaned-up and modified description]
> >> Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
> >> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> >> Acked-by: Vasily Gorbik <gor@linux.ibm.com>
> >> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
> >> ---
> >>  arch/s390/include/asm/cio.h |  1 +
> >>  drivers/s390/cio/chsc.c     | 83 +++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 84 insertions(+)  
> >  
> > (...)
> >  
> >> +/**
> >> + * chsc_scud() - Store control-unit description.
> >> + * @cu:		number of the control-unit
> >> + * @esm:	8 1-byte endpoint security mode values
> >> + * @esm_valid:	validity mask for @esm
> >> + *
> >> + * Interface to retrieve information about the endpoint security
> >> + * modes for up to 8 paths of a control unit.
> >> + *
> >> + * Returns 0 on success.
> >> + */
> >> +int chsc_scud(u16 cu, u64 *esm, u8 *esm_valid)
> >> +{
> >> +	struct chsc_scud *scud = chsc_page;
> >> +	int ret;
> >> +  
> > I'm wondering if it would make sense to check in the chsc
> > characteristics whether that chsc is actually installed (if there's
> > actually a bit for it, although I'd expect so). Some existing chscs
> > check for bits in the characteristics, others don't. (Don't know
> > whether QEMU is the only platform that doesn't provide this chsc.)  
> 
> I don't see any benefit in checking upfront if the CHSC is supported -
> we'll get
> a corresponding CHSC response code and since no error message is logged
> for this
> case, the outcome would be the same as if we checked for the
> characteristics bit
> beforehand.

Yes, that's probably fine, then.

> 
> 
> >> +	spin_lock_irq(&chsc_page_lock);
> >> +	memset(chsc_page, 0, PAGE_SIZE);
> >> +	scud->request.length = SCUD_REQ_LEN;
> >> +	scud->request.code = SCUD_REQ_CMD;
> >> +	scud->fmt = 0;
> >> +	scud->cssid = 0;
> >> +	scud->first_cu = cu;
> >> +	scud->last_cu = cu;
> >> +
> >> +	ret = chsc(scud);
> >> +	if (!ret)
> >> +		ret = chsc_error_from_response(scud->response.code);
> >> +
> >> +	if (!ret && (scud->response.length <= 8 || scud->fmt_resp != 0
> >> +			|| !(scud->cudb[0].flags & 0x80)
> >> +			|| scud->cudb[0].cu != cu)) {
> >> +
> >> +		CIO_MSG_EVENT(2, "chsc: scud failed rc=%04x, L2=%04x "
> >> +			"FMT=%04x, cudb.flags=%02x, cudb.cu=%04x",
> >> +			scud->response.code, scud->response.length,
> >> +			scud->fmt_resp, scud->cudb[0].flags, scud->cudb[0].cu);
> >> +		ret = -EINVAL;
> >> +	}
> >> +
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	memcpy(esm, scud->cudb[0].esm, sizeof(*esm));
> >> +	*esm_valid = scud->cudb[0].esm_valid;
> >> +out:
> >> +	spin_unlock_irq(&chsc_page_lock);
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(chsc_scud);  
> 

FWIW,
Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 08/10] s390/dasd: Display FC Endpoint Security information via sysfs

[Cornelia Huck]

On Wed, 7 Oct 2020 16:33:37 +0200
Jan Höppner <hoeppner@linux.ibm.com> wrote:

> >>>> +static inline void dasd_path_release(struct kobject *kobj)
> >>>> +{
> >>>> +/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
> >>>> +}
> >>>> +    
> >>>
> >>> As already said, I don't think that's a correct way to implement this.
> >>>     
> >>
> >> As you correctly pointed out, our release function doesn't do anything.
> >> This is because our path data is a (static) part of our device.
> >> This data is critical to keep our devices operational.
> >> We can't simply rely on allocated memory if systems are under stress.   
> > 
> > Yes, avoiding freeing and reallocating memory certainly makes sense.
> >   
> >>
> >> Having this data dynamically allocated involves a lot of rework of our
> >> path handling as well. There are a few things that are subject to improvement
> >> and evaluating whether our dasd_path structures can be dynamic is one of
> >> these things. However, even then, the above concern persists and I
> >> highly doubt that dynamic dasd_paths objects are doable for us at this
> >> moment.
> >>
> >> I do understand the concerns, however, we release the memory for dasd_path
> >> structures eventually when dasd_free_device() is called. Until that point,
> >> the data has to be kept alive. The rest is taking care of by the kobject
> >> library.  
> > 
> > Yes, there doesn't seem to be any memory leakage.
> >   
> >> In our path handling we also make sure that we can always verify/validate
> >> paths information even if a system is under high memory pressure. Another
> >> reason why it would contradictory for dasd_path objects to be dynamic.
> >>
> >> I hope this explains the reasoning behind the release function.  
> > 
> > I understand where you're coming from.
> > 
> > However, "static" kobjects (in the sense of "we may re-register the
> > same kobject") are still problematic. Is there any way to simply
> > "disappear" path objects that are not valid at the moment, or mark them
> > as not valid?  
> 
> You could use kobject_del(), but it is rather intended to be used for
> a two-stage removal of the kobject.
> 
> > 
> > Also, the simple act of registering/unregistering a kobject already
> > creates stress from its sysfs interactions... it seems you should try
> > to avoid that as well?
> >   
> 
> We don't re-register kobjects over and over again. The kobjects are
> infact initialized and created only _once_. This is done either during
> device initialization (after dasd_eckd_read_conf() in
> dasd_eckd_check_characteristics()) or when a path is newly added
> (in the path event handler).
> The kobject will stay until the memory for the whole device is being
> freed. This is also the reason why the kobject can stay initialized and
> we track ourselves whether we did the initialization/creation already
> (which we check e.g. when a path is removed and added again).
> So, instead of the release function freeing the kobject data,
> it is done by our dasd_free_device() (same thing, different function IMHO).
> 
> I think the concerns would be more worrisome if we'd remove/add
> the kobjects every time. And then I agree, we'd run into trouble.
> 

The thing that tripped me is

+void dasd_path_remove_kobj(struct dasd_device *device, int chp)
+{
+	if (device->path[chp].in_sysfs) {
+		kobject_put(&device->path[chp].kobj);
+		device->path[chp].in_sysfs = false;
+	}
+}

As an exported function, it is not clear where this may be called from.
Given your explanation above (and some more code reading on my side),
the code looks ok in its current incarnation (but non-idiomatic).

Is there a way to check that indeed nobody re-adds a previously removed
path object due to a (future) programming error? And maybe add a
comment that you must never re-register a path? "The path is gone,
let's remove the object" looks quite tempting.

Re: [PATCH 08/10] s390/dasd: Display FC Endpoint Security information via sysfs

[Jan Höppner]

On 10/7/20 6:40 PM, Cornelia Huck wrote:
> On Wed, 7 Oct 2020 16:33:37 +0200
> Jan Höppner <hoeppner@linux.ibm.com> wrote:
> 
>>>>>> +static inline void dasd_path_release(struct kobject *kobj)
>>>>>> +{
>>>>>> +/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
>>>>>> +}
>>>>>> +    
>>>>>
>>>>> As already said, I don't think that's a correct way to implement this.
>>>>>     
>>>>
>>>> As you correctly pointed out, our release function doesn't do anything.
>>>> This is because our path data is a (static) part of our device.
>>>> This data is critical to keep our devices operational.
>>>> We can't simply rely on allocated memory if systems are under stress.   
>>>
>>> Yes, avoiding freeing and reallocating memory certainly makes sense.
>>>   
>>>>
>>>> Having this data dynamically allocated involves a lot of rework of our
>>>> path handling as well. There are a few things that are subject to improvement
>>>> and evaluating whether our dasd_path structures can be dynamic is one of
>>>> these things. However, even then, the above concern persists and I
>>>> highly doubt that dynamic dasd_paths objects are doable for us at this
>>>> moment.
>>>>
>>>> I do understand the concerns, however, we release the memory for dasd_path
>>>> structures eventually when dasd_free_device() is called. Until that point,
>>>> the data has to be kept alive. The rest is taking care of by the kobject
>>>> library.  
>>>
>>> Yes, there doesn't seem to be any memory leakage.
>>>   
>>>> In our path handling we also make sure that we can always verify/validate
>>>> paths information even if a system is under high memory pressure. Another
>>>> reason why it would contradictory for dasd_path objects to be dynamic.
>>>>
>>>> I hope this explains the reasoning behind the release function.  
>>>
>>> I understand where you're coming from.
>>>
>>> However, "static" kobjects (in the sense of "we may re-register the
>>> same kobject") are still problematic. Is there any way to simply
>>> "disappear" path objects that are not valid at the moment, or mark them
>>> as not valid?  
>>
>> You could use kobject_del(), but it is rather intended to be used for
>> a two-stage removal of the kobject.
>>
>>>
>>> Also, the simple act of registering/unregistering a kobject already
>>> creates stress from its sysfs interactions... it seems you should try
>>> to avoid that as well?
>>>   
>>
>> We don't re-register kobjects over and over again. The kobjects are
>> infact initialized and created only _once_. This is done either during
>> device initialization (after dasd_eckd_read_conf() in
>> dasd_eckd_check_characteristics()) or when a path is newly added
>> (in the path event handler).
>> The kobject will stay until the memory for the whole device is being
>> freed. This is also the reason why the kobject can stay initialized and
>> we track ourselves whether we did the initialization/creation already
>> (which we check e.g. when a path is removed and added again).
>> So, instead of the release function freeing the kobject data,
>> it is done by our dasd_free_device() (same thing, different function IMHO).
>>
>> I think the concerns would be more worrisome if we'd remove/add
>> the kobjects every time. And then I agree, we'd run into trouble.
>>
> 
> The thing that tripped me is
> 
> +void dasd_path_remove_kobj(struct dasd_device *device, int chp)
> +{
> +	if (device->path[chp].in_sysfs) {
> +		kobject_put(&device->path[chp].kobj);
> +		device->path[chp].in_sysfs = false;
> +	}
> +}
> 
> As an exported function, it is not clear where this may be called from.
> Given your explanation above (and some more code reading on my side),
> the code looks ok in its current incarnation (but non-idiomatic).
> 
> Is there a way to check that indeed nobody re-adds a previously removed
> path object due to a (future) programming error? And maybe add a
> comment that you must never re-register a path? "The path is gone,
> let's remove the object" looks quite tempting.
> 

A comment is the minimum I can think of at the moment and
I'll prepare a fixup patch or a new version of this patch that adds
a proper comment for this function.
Other ways to protect the usage must be investigated. 
I have to discuss with Stefan what the best approach would be as the patchset
is supposed to be ready for upstream integration.

I'd prefer a fixup patch that we could send with at least one more fixup patch
that we have in the pipe already. Let's see. I hope that's fine with you
(and Jens obviously) so far.

regards,
Jan

Re: [PATCH 08/10] s390/dasd: Display FC Endpoint Security information via sysfs

[Cornelia Huck]

On Wed, 7 Oct 2020 22:10:11 +0200
Jan Höppner <hoeppner@linux.ibm.com> wrote:

> On 10/7/20 6:40 PM, Cornelia Huck wrote:
> > On Wed, 7 Oct 2020 16:33:37 +0200
> > Jan Höppner <hoeppner@linux.ibm.com> wrote:
> >   
> >>>>>> +static inline void dasd_path_release(struct kobject *kobj)
> >>>>>> +{
> >>>>>> +/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
> >>>>>> +}
> >>>>>> +      
> >>>>>
> >>>>> As already said, I don't think that's a correct way to implement this.
> >>>>>       
> >>>>
> >>>> As you correctly pointed out, our release function doesn't do anything.
> >>>> This is because our path data is a (static) part of our device.
> >>>> This data is critical to keep our devices operational.
> >>>> We can't simply rely on allocated memory if systems are under stress.     
> >>>
> >>> Yes, avoiding freeing and reallocating memory certainly makes sense.
> >>>     
> >>>>
> >>>> Having this data dynamically allocated involves a lot of rework of our
> >>>> path handling as well. There are a few things that are subject to improvement
> >>>> and evaluating whether our dasd_path structures can be dynamic is one of
> >>>> these things. However, even then, the above concern persists and I
> >>>> highly doubt that dynamic dasd_paths objects are doable for us at this
> >>>> moment.
> >>>>
> >>>> I do understand the concerns, however, we release the memory for dasd_path
> >>>> structures eventually when dasd_free_device() is called. Until that point,
> >>>> the data has to be kept alive. The rest is taking care of by the kobject
> >>>> library.    
> >>>
> >>> Yes, there doesn't seem to be any memory leakage.
> >>>     
> >>>> In our path handling we also make sure that we can always verify/validate
> >>>> paths information even if a system is under high memory pressure. Another
> >>>> reason why it would contradictory for dasd_path objects to be dynamic.
> >>>>
> >>>> I hope this explains the reasoning behind the release function.    
> >>>
> >>> I understand where you're coming from.
> >>>
> >>> However, "static" kobjects (in the sense of "we may re-register the
> >>> same kobject") are still problematic. Is there any way to simply
> >>> "disappear" path objects that are not valid at the moment, or mark them
> >>> as not valid?    
> >>
> >> You could use kobject_del(), but it is rather intended to be used for
> >> a two-stage removal of the kobject.
> >>  
> >>>
> >>> Also, the simple act of registering/unregistering a kobject already
> >>> creates stress from its sysfs interactions... it seems you should try
> >>> to avoid that as well?
> >>>     
> >>
> >> We don't re-register kobjects over and over again. The kobjects are
> >> infact initialized and created only _once_. This is done either during
> >> device initialization (after dasd_eckd_read_conf() in
> >> dasd_eckd_check_characteristics()) or when a path is newly added
> >> (in the path event handler).
> >> The kobject will stay until the memory for the whole device is being
> >> freed. This is also the reason why the kobject can stay initialized and
> >> we track ourselves whether we did the initialization/creation already
> >> (which we check e.g. when a path is removed and added again).
> >> So, instead of the release function freeing the kobject data,
> >> it is done by our dasd_free_device() (same thing, different function IMHO).
> >>
> >> I think the concerns would be more worrisome if we'd remove/add
> >> the kobjects every time. And then I agree, we'd run into trouble.
> >>  
> > 
> > The thing that tripped me is
> > 
> > +void dasd_path_remove_kobj(struct dasd_device *device, int chp)
> > +{
> > +	if (device->path[chp].in_sysfs) {
> > +		kobject_put(&device->path[chp].kobj);
> > +		device->path[chp].in_sysfs = false;
> > +	}
> > +}
> > 
> > As an exported function, it is not clear where this may be called from.
> > Given your explanation above (and some more code reading on my side),
> > the code looks ok in its current incarnation (but non-idiomatic).
> > 
> > Is there a way to check that indeed nobody re-adds a previously removed
> > path object due to a (future) programming error? And maybe add a
> > comment that you must never re-register a path? "The path is gone,
> > let's remove the object" looks quite tempting.
> >   
> 
> A comment is the minimum I can think of at the moment and
> I'll prepare a fixup patch or a new version of this patch that adds
> a proper comment for this function.
> Other ways to protect the usage must be investigated. 
> I have to discuss with Stefan what the best approach would be as the patchset
> is supposed to be ready for upstream integration.
> 
> I'd prefer a fixup patch that we could send with at least one more fixup patch
> that we have in the pipe already. Let's see. I hope that's fine with you
> (and Jens obviously) so far.

Fine with me. I don't really have a horse in that race; I just wanted
to look at this from a vfio-ccw perspective and then stumbled over the
kobject handling...

Re: [PATCH 08/10] s390/dasd: Display FC Endpoint Security information via sysfs

[Stefan Haberland]

Am 08.10.20 um 09:03 schrieb Cornelia Huck:
> On Wed, 7 Oct 2020 22:10:11 +0200
> Jan Höppner <hoeppner@linux.ibm.com> wrote:
>
>> On 10/7/20 6:40 PM, Cornelia Huck wrote:
>>> On Wed, 7 Oct 2020 16:33:37 +0200
>>> Jan Höppner <hoeppner@linux.ibm.com> wrote:
>>>   
>>>>>>>> +static inline void dasd_path_release(struct kobject *kobj)
>>>>>>>> +{
>>>>>>>> +/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
>>>>>>>> +}
>>>>>>>> +      
>>>>>>> As already said, I don't think that's a correct way to implement this.
>>>>>>>       
>>>>>> As you correctly pointed out, our release function doesn't do anything.
>>>>>> This is because our path data is a (static) part of our device.
>>>>>> This data is critical to keep our devices operational.
>>>>>> We can't simply rely on allocated memory if systems are under stress.     
>>>>> Yes, avoiding freeing and reallocating memory certainly makes sense.
>>>>>     
>>>>>> Having this data dynamically allocated involves a lot of rework of our
>>>>>> path handling as well. There are a few things that are subject to improvement
>>>>>> and evaluating whether our dasd_path structures can be dynamic is one of
>>>>>> these things. However, even then, the above concern persists and I
>>>>>> highly doubt that dynamic dasd_paths objects are doable for us at this
>>>>>> moment.
>>>>>>
>>>>>> I do understand the concerns, however, we release the memory for dasd_path
>>>>>> structures eventually when dasd_free_device() is called. Until that point,
>>>>>> the data has to be kept alive. The rest is taking care of by the kobject
>>>>>> library.    
>>>>> Yes, there doesn't seem to be any memory leakage.
>>>>>     
>>>>>> In our path handling we also make sure that we can always verify/validate
>>>>>> paths information even if a system is under high memory pressure. Another
>>>>>> reason why it would contradictory for dasd_path objects to be dynamic.
>>>>>>
>>>>>> I hope this explains the reasoning behind the release function.    
>>>>> I understand where you're coming from.
>>>>>
>>>>> However, "static" kobjects (in the sense of "we may re-register the
>>>>> same kobject") are still problematic. Is there any way to simply
>>>>> "disappear" path objects that are not valid at the moment, or mark them
>>>>> as not valid?    
>>>> You could use kobject_del(), but it is rather intended to be used for
>>>> a two-stage removal of the kobject.
>>>>  
>>>>> Also, the simple act of registering/unregistering a kobject already
>>>>> creates stress from its sysfs interactions... it seems you should try
>>>>> to avoid that as well?
>>>>>     
>>>> We don't re-register kobjects over and over again. The kobjects are
>>>> infact initialized and created only _once_. This is done either during
>>>> device initialization (after dasd_eckd_read_conf() in
>>>> dasd_eckd_check_characteristics()) or when a path is newly added
>>>> (in the path event handler).
>>>> The kobject will stay until the memory for the whole device is being
>>>> freed. This is also the reason why the kobject can stay initialized and
>>>> we track ourselves whether we did the initialization/creation already
>>>> (which we check e.g. when a path is removed and added again).
>>>> So, instead of the release function freeing the kobject data,
>>>> it is done by our dasd_free_device() (same thing, different function IMHO).
>>>>
>>>> I think the concerns would be more worrisome if we'd remove/add
>>>> the kobjects every time. And then I agree, we'd run into trouble.
>>>>  
>>> The thing that tripped me is
>>>
>>> +void dasd_path_remove_kobj(struct dasd_device *device, int chp)
>>> +{
>>> +	if (device->path[chp].in_sysfs) {
>>> +		kobject_put(&device->path[chp].kobj);
>>> +		device->path[chp].in_sysfs = false;
>>> +	}
>>> +}
>>>
>>> As an exported function, it is not clear where this may be called from.
>>> Given your explanation above (and some more code reading on my side),
>>> the code looks ok in its current incarnation (but non-idiomatic).
>>>
>>> Is there a way to check that indeed nobody re-adds a previously removed
>>> path object due to a (future) programming error? And maybe add a
>>> comment that you must never re-register a path? "The path is gone,
>>> let's remove the object" looks quite tempting.
>>>   
>> A comment is the minimum I can think of at the moment and
>> I'll prepare a fixup patch or a new version of this patch that adds
>> a proper comment for this function.
>> Other ways to protect the usage must be investigated. 
>> I have to discuss with Stefan what the best approach would be as the patchset
>> is supposed to be ready for upstream integration.
>>
>> I'd prefer a fixup patch that we could send with at least one more fixup patch
>> that we have in the pipe already. Let's see. I hope that's fine with you
>> (and Jens obviously) so far.
> Fine with me. I don't really have a horse in that race; I just wanted
> to look at this from a vfio-ccw perspective and then stumbled over the
> kobject handling...
>

Thanks for this. I will send a v2 shortly.